korlov42 commented on code in PR #7449:
URL: https://github.com/apache/ignite-3/pull/7449#discussion_r2712122622
##########
modules/client-handler/src/main/java/org/apache/ignite/client/handler/requests/tx/ClientTransactionRollbackRequest.java:
##########
@@ -70,6 +73,15 @@ public static CompletableFuture<ResponseWriter> process(
merge(table.internalTable(), partId, consistentId, token,
tx, false);
}
}
+
+ if (cnt > 0) {
+ in.unpackLong(); // Unpack causality.
+
+ ReadWriteTransactionImpl tx0 = (ReadWriteTransactionImpl) tx;
+
+ // Enforce cleanup.
+ tx0.noRemoteWrites(sendRemoteWritesFlag && in.unpackBoolean());
Review Comment:
does it make sense to define `noRemoteWrites` on `InternalTransaction`
interface? I understand that it doesn't make much sense for RO transaction to
specify this, but, unfortunately, we don't have exhaustive test coverage to
spot a problem in timely manner when (or `if`) new type of non-RO transaction
will be added and we potentially may end up with ClassCast here
##########
modules/client-handler/src/main/java/org/apache/ignite/client/handler/ClientHandlerModule.java:
##########
@@ -98,7 +98,8 @@ public class ClientHandlerModule implements IgniteComponent,
PlatformComputeTran
ProtocolBitmaskFeature.SQL_DIRECT_TX_MAPPING,
ProtocolBitmaskFeature.TX_CLIENT_GETALL_SUPPORTS_TX_OPTIONS,
ProtocolBitmaskFeature.SQL_MULTISTATEMENT_SUPPORT,
- ProtocolBitmaskFeature.COMPUTE_OBSERVABLE_TS
+ ProtocolBitmaskFeature.COMPUTE_OBSERVABLE_TS,
+ ProtocolBitmaskFeature.TX_DIRECT_MAPPING_SEND_REMOTE_WRITES
Review Comment:
does it make sense to disable direct mapping for clients which doesn't
support this feature?
##########
modules/client-handler/src/main/java/org/apache/ignite/client/handler/requests/tx/ClientTransactionCommitRequest.java:
##########
@@ -83,8 +86,13 @@ public static CompletableFuture<ResponseWriter> process(
if (cnt > 0) {
long causality = in.unpackLong();
- // Update causality.
+ // Update causality. Used to assign commit timestamp after all
enlistments.
clockService.updateClock(HybridTimestamp.hybridTimestamp(causality));
+
+ ReadWriteTransactionImpl tx0 = (ReadWriteTransactionImpl) tx;
+
+ // Enforce cleanup.
+ tx0.noRemoteWrites(sendRemoteWritesFlag && in.unpackBoolean());
Review Comment:
this flag is written unconditionally on client, but read only if there is at
least one enlistment. I believe this makes it error prone when more fields will
be added in the future
--
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]
