ibessonov commented on code in PR #6309:
URL: https://github.com/apache/ignite-3/pull/6309#discussion_r2227622538
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/replicator/PartitionReplicaListener.java:
##########
@@ -891,27 +891,37 @@ private CompletableFuture<List<BinaryRow>>
retrieveExactEntriesUntilCursorEmpty(
)
).cursor();
- var resolutionFuts = new
ArrayList<CompletableFuture<TimedBinaryRow>>(count);
+ var rows = new ArrayList<BinaryRow>(count);
- while (resolutionFuts.size() < count && cursor.hasNext()) {
+ var resolutionFuts = new
ArrayList<CompletableFuture<TimedBinaryRow>>();
+
+ while ((rows.size() + resolutionFuts.size()) < count &&
cursor.hasNext()) {
ReadResult readResult = cursor.next();
- HybridTimestamp newestCommitTimestamp =
readResult.newestCommitTimestamp();
- TimedBinaryRow candidate;
- if (newestCommitTimestamp == null || !readResult.isWriteIntent()) {
- candidate = null;
+ BinaryRow row = readResult.binaryRow();
+ UUID retrievedResultTxId = readResult.transactionId();
+
+ if (!readResult.isWriteIntent() || (readTimestamp == null &&
txId.equals(retrievedResultTxId))) {
+ if (row != null) {
+ rows.add(row);
Review Comment:
The order of rows retrieved from the cursor will be violated if there are
write intents. Old behavior is not preserved, it might be dangerous
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/replicator/PartitionReplicaListener.java:
##########
@@ -891,27 +891,37 @@ private CompletableFuture<List<BinaryRow>>
retrieveExactEntriesUntilCursorEmpty(
)
).cursor();
- var resolutionFuts = new
ArrayList<CompletableFuture<TimedBinaryRow>>(count);
+ var rows = new ArrayList<BinaryRow>(count);
- while (resolutionFuts.size() < count && cursor.hasNext()) {
+ var resolutionFuts = new
ArrayList<CompletableFuture<TimedBinaryRow>>();
+
+ while ((rows.size() + resolutionFuts.size()) < count &&
cursor.hasNext()) {
ReadResult readResult = cursor.next();
- HybridTimestamp newestCommitTimestamp =
readResult.newestCommitTimestamp();
- TimedBinaryRow candidate;
- if (newestCommitTimestamp == null || !readResult.isWriteIntent()) {
- candidate = null;
+ BinaryRow row = readResult.binaryRow();
+ UUID retrievedResultTxId = readResult.transactionId();
+
+ if (!readResult.isWriteIntent() || (readTimestamp == null &&
txId.equals(retrievedResultTxId))) {
+ if (row != null) {
+ rows.add(row);
+ }
} else {
- BinaryRow committedRow =
cursor.committed(newestCommitTimestamp);
+ HybridTimestamp newestCommitTimestamp =
readResult.newestCommitTimestamp();
- candidate = committedRow == null ? null : new
TimedBinaryRow(committedRow, newestCommitTimestamp);
- }
+ TimedBinaryRow candidate;
+ if (newestCommitTimestamp == null) {
+ candidate = null;
+ } else {
+ BinaryRow committedRow =
cursor.committed(newestCommitTimestamp);
Review Comment:
I want to share my thoughts for the future. Calling
`cursor.committed(newestCommitTimestamp)` outside of the `() -> candidate`
leads to performance degradation if we encounter a lot of write intents. Maybe
we need to add a TODO
--
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
