ascherbakoff commented on code in PR #4929:
URL: https://github.com/apache/ignite-3/pull/4929#discussion_r1898420260
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/HybridTimestampTracker.java:
##########
@@ -17,36 +17,102 @@
package org.apache.ignite.internal.tx;
-import static
org.apache.ignite.internal.hlc.HybridTimestamp.NULL_HYBRID_TIMESTAMP;
+import static
org.apache.ignite.internal.hlc.HybridTimestamp.hybridTimestampToLong;
+import static
org.apache.ignite.internal.hlc.HybridTimestamp.nullableHybridTimestamp;
import java.util.concurrent.atomic.AtomicLong;
+import java.util.function.Consumer;
import org.apache.ignite.internal.hlc.HybridTimestamp;
import org.jetbrains.annotations.Nullable;
/**
- * Hybrid timestamp tracker.
+ * Interface is used to provide a track timestamp into a transaction operation.
*/
-public class HybridTimestampTracker {
- /** Timestamp. */
- private final AtomicLong timestamp = new AtomicLong(NULL_HYBRID_TIMESTAMP);
+public abstract class HybridTimestampTracker {
Review Comment:
Why this is not the interface ?
##########
modules/client-handler/src/main/java/org/apache/ignite/client/handler/ClientInboundMessageHandler.java:
##########
@@ -985,17 +1016,26 @@ private static Object
unpackExtensionValue(HandshakeExtension handshakeExtension
return null;
}
+ /**
+ * Gets an observation timestamp for the operation being processed or
{@link HybridTimestamp#MIN_VALUE} if the timestamp was not defined
+ * by the operation.
+ * The method returns a current timestamp for the handshake operation.
+ *
+ * @param out Output message packer.
+ * @return A long representation of the observation timestamp.
+ */
private long observableTimestamp(@Nullable ClientMessagePacker out) {
- // Certain operations can override the timestamp and provide it in the
meta object.
- if (out != null) {
- Object meta = out.meta();
+ if (out == null) {
+ return clockService.currentLong();
Review Comment:
Still not clear why it's needed on handshake.
Query just should load latest table definitions and use them,
##########
modules/client-handler/src/main/java/org/apache/ignite/client/handler/ClientInboundMessageHandler.java:
##########
@@ -801,13 +822,23 @@ private void processOperation(ChannelHandlerContext ctx,
ClientMessageUnpacker i
return ClientJdbcFinishTxRequest.process(in, out,
jdbcQueryEventHandler);
case ClientOp.SQL_EXEC_SCRIPT:
- return ClientSqlExecuteScriptRequest.process(in,
queryProcessor, igniteTransactions);
+ return ClientSqlExecuteScriptRequest.process(in,
queryProcessor).thenRun(() -> {
+ // TODO: IGNITE-24055 Observation timestamp must be
updated only for DDL "CREATE TABLE..."
+ if (!(out.meta() instanceof HybridTimestamp)) {
+ out.meta(clockService.current());
+ }
+ });
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..."
Review Comment:
But the comment says otherwise: Observation timestamp must be updated only ..
This is confusing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
