sanpwc commented on code in PR #3069:
URL: https://github.com/apache/ignite-3/pull/3069#discussion_r1466385511
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/storage/InternalTableImpl.java:
##########
@@ -1348,7 +1349,8 @@ private Publisher<BinaryRow> scan(
return replicaSvc.invoke(recipientNode, request);
},
// TODO: IGNITE-17666 Close cursor tx finish.
- (unused, fut) -> fut);
+ (commit, fut) -> onScanComplete(txId, partGroupId, fut,
recipientNode, commit)
Review Comment:
It's not the only place where we create ScanPublisher, why we prepare
onClose closure that will send cursor close request only here? Or in other
words, why we call onScanComplete only here and not within publishers on lines
1394 and 1442?
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/storage/InternalTableImpl.java:
##########
@@ -1459,7 +1461,39 @@ private Publisher<BinaryRow> scan(
return replicaSvc.invoke(recipient.node(), request);
},
// TODO: IGNITE-17666 Close cursor tx finish.
- (unused, fut) -> fut);
+ (unused, fut) -> fut.thenApply(cursorId -> null));
+ }
+
+ /**
+ * Closes the cursor on server side.
+ *
+ * @param txId Transaction id.
+ * @param replicaGrpId Replication group id.
+ * @param scanIdFut Future to scan id.
+ * @param recipientNode Server node where the scan was started.
+ * @param commit Commit flag, when the flag is {@code null} the scan was
closed manually.
+ * @return The future.
+ */
+ private CompletableFuture<Void> onScanComplete(
+ UUID txId,
+ ReplicationGroupId replicaGrpId,
+ CompletableFuture<Long> scanIdFut,
+ ClusterNode recipientNode,
+ Boolean commit
+ ) {
+ return scanIdFut.thenCompose(scanId -> {
+ if (commit) {
Review Comment:
Despite the fact that commit is now Boolean and you explicitly say that
it'll be null if the scan was closed manually, you do not check whether it's
null or not. Please also check the comment above.
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/storage/InternalTableImpl.java:
##########
@@ -1459,7 +1461,39 @@ private Publisher<BinaryRow> scan(
return replicaSvc.invoke(recipient.node(), request);
},
// TODO: IGNITE-17666 Close cursor tx finish.
- (unused, fut) -> fut);
+ (unused, fut) -> fut.thenApply(cursorId -> null));
+ }
+
+ /**
+ * Closes the cursor on server side.
+ *
+ * @param txId Transaction id.
+ * @param replicaGrpId Replication group id.
+ * @param scanIdFut Future to scan id.
+ * @param recipientNode Server node where the scan was started.
+ * @param commit Commit flag, when the flag is {@code null} the scan was
closed manually.
+ * @return The future.
+ */
+ private CompletableFuture<Void> onScanComplete(
Review Comment:
I'd rather say completeScan instead of onScanComplete.
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/storage/InternalTableImpl.java:
##########
@@ -1459,7 +1461,39 @@ private Publisher<BinaryRow> scan(
return replicaSvc.invoke(recipient.node(), request);
},
// TODO: IGNITE-17666 Close cursor tx finish.
- (unused, fut) -> fut);
+ (unused, fut) -> fut.thenApply(cursorId -> null));
+ }
+
+ /**
+ * Closes the cursor on server side.
+ *
+ * @param txId Transaction id.
+ * @param replicaGrpId Replication group id.
+ * @param scanIdFut Future to scan id.
+ * @param recipientNode Server node where the scan was started.
+ * @param commit Commit flag, when the flag is {@code null} the scan was
closed manually.
Review Comment:
> scanIdFut @param commit Commit flag, when the flag is {@code null} the
scan was closed manually.
Parameter naming and javadoc is confusing? Please rename it to
sendCursorCloseRequest or similar.
Why it's Boolean and not boolean?
Where do we set null?
Did you check `cancel` evaluation? Do we propagate correct values in all
cases?
--
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]
