ascherbakoff commented on code in PR #4929:
URL: https://github.com/apache/ignite-3/pull/4929#discussion_r1897405210
##########
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..."
+ if (!(out.meta() instanceof HybridTimestamp)) {
Review Comment:
Looks like meta() value is ignored if it's not a timestamp, so it makes no
sense to set it.
So why it's not
`if (out.meta() == null) ?`
##########
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:
It doesn't make sense to pass current server time on handshake to client.
##########
modules/client-handler/src/main/java/org/apache/ignite/client/handler/requests/table/ClientTableCommon.java:
##########
@@ -420,27 +421,51 @@ public static TableNotFoundException
tableIdNotFoundException(Integer tableId) {
* @param in Unpacker.
* @param out Packer.
* @param resources Resource registry.
- * @param igniteTransactions Ignite transactions.
+ * @param txManager Ignite transactions.
* @param readOnly Read only flag.
* @return Transaction.
*/
public static @Nullable InternalTransaction readOrStartImplicitTx(
ClientMessageUnpacker in,
ClientMessagePacker out,
ClientResourceRegistry resources,
- IgniteTransactionsImpl igniteTransactions,
+ TxManager txManager,
boolean readOnly
) {
- var tx = readTx(in, out, resources);
+ InternalTransaction tx = readTx(in, out, resources);
if (tx == null) {
- tx = igniteTransactions.beginImplicit(readOnly);
+ tx = startTx(out, txManager, null, true, readOnly);
Review Comment:
In case of implicit transaction observable ts must be transferred from the
client in the message.
It doesn't happen now.
##########
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:
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.
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/HybridTimestampTrackerImpl.java:
##########
@@ -0,0 +1,54 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.tx;
+
+import static
org.apache.ignite.internal.hlc.HybridTimestamp.NULL_HYBRID_TIMESTAMP;
+
+import java.util.concurrent.atomic.AtomicLong;
+import org.apache.ignite.internal.hlc.HybridTimestamp;
+import org.jetbrains.annotations.Nullable;
+
+/**
+ * Hybrid timestamp tracker.
+ */
+public class HybridTimestampTrackerImpl implements HybridTimestampTracker {
Review 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);
}
}
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
