IGNITE-23305 Get rid of client HybridTimestampTracker #4929
Conversation
vldpyatkov
force-pushed
the
ignite-23305
branch
from
3123be1 to
d60b660
Compare
modules/runner/src/main/java/org/apache/ignite/internal/app/IgniteImpl.java
Show resolved
Hide resolved
...ient-handler/src/main/java/org/apache/ignite/client/handler/ClientInboundMessageHandler.java
Show resolved
Hide resolved
...ient-handler/src/main/java/org/apache/ignite/client/handler/ClientInboundMessageHandler.java
Show resolved
Hide resolved
...sactions/src/main/java/org/apache/ignite/internal/tx/impl/IgniteAbstractTransactionImpl.java
Outdated
Show resolved
Hide resolved
...es/transactions/src/main/java/org/apache/ignite/internal/tx/ObservableTimestampProvider.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/apache/ignite/client/handler/requests/sql/ClientSqlCursorCloseRequest.java
Outdated
Show resolved
Hide resolved
...client-handler/src/main/java/org/apache/ignite/client/handler/JdbcQueryEventHandlerImpl.java
Outdated
Show resolved
Hide resolved
| @@ -35,14 +35,14 @@ | |||
| public class IgniteTransactionsImpl implements IgniteTransactions { | |||
| private final TxManager txManager; | |||
|
|
|||
| private final HybridTimestampTracker observableTimestampTracker; | |||
| private final HybridTimestampTrackerImpl observableTimestampTracker; | |||
There was a problem hiding this comment.
let's use interface instead of implementation. Besides, there are number of methods which are not used anymore. Let's clean up IgniteTransactionsImpl
There was a problem hiding this comment.
In this instance, the field can only be an instance of HybridTimestampTrackerImpl.
It is not worth overdoing with the use of interfaces.
There was a problem hiding this comment.
there are number of methods which are not used anymore
Good point.
| @@ -362,7 +362,7 @@ public class TableManager implements IgniteTablesInternal, IgniteComponent { | |||
|
|
|||
| private final LowWatermark lowWatermark; | |||
|
|
|||
| private final HybridTimestampTracker observableTimestampTracker; | |||
| private final HybridTimestampTrackerImpl observableTimestampTracker; | |||
There was a problem hiding this comment.
let's use interface here and below. Please check this kind of changes everywhere in your patch
There was a problem hiding this comment.
I prefer to use a concrete implementation until the other is not needed.
...ient-handler/src/main/java/org/apache/ignite/client/handler/ClientInboundMessageHandler.java
Show resolved
Hide resolved
|
|
||
| case ClientOp.SQL_QUERY_META: | ||
| return ClientSqlQueryMetadataRequest.process(in, out, queryProcessor, resources); | ||
|
|
||
| case ClientOp.SQL_EXEC_BATCH: | ||
| return ClientSqlExecuteBatchRequest.process(in, out, queryProcessor, resources, igniteTransactions); | ||
| return ClientSqlExecuteBatchRequest.process(in, out, queryProcessor, resources).thenRun(() -> { | ||
| // TODO: IGNITE-24055 Observation timestamp must be updated only for DDL "CREATE TABLE..." |
There was a problem hiding this comment.
Can you provide detailed description in the ticket which SQL statements requre passing back observable ts and which not, and fix the comment accordingly ?
I believe it's not only DDL "CREATE TABLE..."
This affects all places with the TODO.
There was a problem hiding this comment.
Unfortunately, I do not have a full list of SQL operations.
We should do an investigation before taking this issue for implementation. It is just a marker for the SQL team.
| if (out != null) { | ||
| Object meta = out.meta(); | ||
| if (out == null) { | ||
| return clockService.currentLong(); |
There was a problem hiding this comment.
It doesn't make sense to pass current server time on handshake to client.
There was a problem hiding this comment.
But we have tests where we consider it.
I mean the test where we make a schema (create table, zones) and then start a client, which has to execute a query over this schema.
|
|
||
| if (tx == null) { | ||
| tx = igniteTransactions.beginImplicit(readOnly); | ||
| tx = startTx(out, txManager, null, true, readOnly); |
There was a problem hiding this comment.
In case of implicit transaction observable ts must be transferred from the client in the message.
It doesn't happen now.
There was a problem hiding this comment.
Observation timestamp is not necessary for the implicit transaction.
I will add a comment.
| /** | ||
| * Hybrid timestamp tracker. | ||
| */ | ||
| public class HybridTimestampTrackerImpl implements HybridTimestampTracker { |
There was a problem hiding this comment.
You don't need this change. It brings a lot of modified files in PR without any profit.
Instead, change implementation in the following way:
public class HybridTimestampTracker {
/** Timestamp. */
private final AtomicLong timestamp = new AtomicLong(NULL_HYBRID_TIMESTAMP);
private final Consumer<HybridTimestamp> updateTs;
public HybridTimestampTracker() {
this.updateTs = timestamp -> {
long tsVal = HybridTimestamp.hybridTimestampToLong(ts);
HybridTimestampTracker.this.timestamp.updateAndGet(x -> Math.max(x, tsVal));
};
}
public HybridTimestampTracker(@Nullable HybridTimestamp currentTs, Consumer<HybridTimestamp> updateTs) {
this.timestamp.set(currentTs.longValue());
this.updateTs = updateTs;
}
public @Nullable HybridTimestamp get() {
return HybridTimestamp.nullableHybridTimestamp(timestamp.get());
}
public void update(@Nullable HybridTimestamp ts) {
updateTs.accept(ts);
}
}
https://issues.apache.org/jira/browse/IGNITE-23305