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
| } | ||
|
|
||
| return clockService.currentLong(); | ||
| return HybridTimestamp.MIN_VALUE.longValue(); |
There was a problem hiding this comment.
I suggest to rework a bit this method to see the code branching more clearly.
private long observableTimestamp(@Nullable ClientMessagePacker out) {
// Certain operations can override the timestamp and provide it in the meta object.
if (out == null) {
return clockService.currentLong();
}
Object meta = out.meta();
if (meta instanceof HybridTimestamp) {
return ((HybridTimestamp) meta).longValue();
}
return HybridTimestamp.MIN_VALUE.longValue();
}
| public IgniteAbstractTransactionImpl(TxManager txManager, UUID id, UUID coordinatorId, boolean implicit) { | ||
| public IgniteAbstractTransactionImpl( | ||
| TxManager txManager, | ||
| ObservableTimestampProvider observableTsTracker, |
| /** | ||
| * Interface is used to provide an observable timestamp into a transaction operation. | ||
| */ | ||
| public interface ObservableTimestampProvider { |
There was a problem hiding this comment.
it's better to name it HybridTimestampTracker and rename implementation to HybridTimestampTrackerImpl so w will avoid hundreds of meaningless changes. Besides, I believe HybridTimestampTracker is better fit for such interface
| * @return Future representing result of operation. | ||
| */ | ||
| public static CompletableFuture<Void> process( | ||
| ClientMessageUnpacker in, | ||
| ClientMessagePacker out, | ||
| ClientResourceRegistry resources, | ||
| IgniteTransactionsImpl transactions | ||
| ClientResourceRegistry resources | ||
| ) throws IgniteInternalCheckedException { | ||
| long resourceId = in.unpackLong(); | ||
|
|
||
| ClientSqlResultSet asyncResultSet = resources.remove(resourceId).get(ClientSqlResultSet.class); | ||
|
|
||
| return asyncResultSet.closeAsync() | ||
| .thenApply(ignored -> { |
There was a problem hiding this comment.
you can remove thenApply entirely
| * Observation timestamp tracker. | ||
| * TODO: IGNITE-24053 Remove this after the issue will be fixed. | ||
| * */ | ||
| private final HybridTimestampTracker timestampTracker = new HybridTimestampTracker(); |
There was a problem hiding this comment.
I've checked the mentioned JIRA, and it doesn't seem correct. At the moment, JDBC driver doesn't track time on client's side, and driver cannot connect to more than one server. During connection, server issues connectionId for every java.sql.Connection instance, and every request from the connection provides connectionId.
Given that said, we need to create one tracker per JdbcConnectionContext and use it for every request within this connection. Changes in JDBC-related requests processing methods are not necessary since driver doesn't track observable time
There was a problem hiding this comment.
Why does the JDBC driver cannot track an observation timestamp?
There was a problem hiding this comment.
because it doesn't need to. There is no support for multi-node connections yet (simultaneous connections to several servers within the same cluster), so why should we bother?
https://issues.apache.org/jira/browse/IGNITE-23305