rpuch commented on code in PR #5242:
URL: https://github.com/apache/ignite-3/pull/5242#discussion_r1959256378
##########
modules/partition-replicator/src/main/java/org/apache/ignite/internal/partition/replicator/raft/snapshot/outgoing/OutgoingSnapshot.java:
##########
@@ -515,14 +563,20 @@ public boolean alreadyPassed(int tableId, RowId rowId) {
assert mvOperationsLock.isLocked() : "MV operations lock must be
acquired!";
if (mvPartitionDeliveryState == null) {
- // We haven't started sending MV data yet.
+ // We haven't even frozen the snapshot scope yet.
return false;
}
- if (finishedMvData()) {
+ // First, check if all MV data is delivered, because it can also be
empty.
+ if (mvPartitionDeliveryState.isExhausted() ||
!mvPartitionDeliveryState.containsTableId(tableId)) {
Review Comment:
I'm not sure `containsTableId()` check looks good here as this method is
about an already passed (that is, already sent) piece of data, but if tableId
is not contained, it is simply not relevant.
Can this check be made out of this method, before calling `alreadyPassed()`?
If not, maybe we should rename the method?
##########
modules/partition-replicator/src/main/java/org/apache/ignite/internal/partition/replicator/raft/snapshot/outgoing/OutgoingSnapshot.java:
##########
@@ -100,30 +104,42 @@ public class OutgoingSnapshot {
private volatile PartitionSnapshotMeta frozenMeta;
/**
- * {@link RowId}s for which the corresponding rows were sent out of order
(relative to the order in which this
- * snapshot sends rows), hence they must be skipped when sending rows
normally.
+ * {@link RowId}s for which the corresponding rows were sent out of order
(relative to the order in which this snapshot sends rows),
+ * hence they must be skipped when sending rows normally.
+ *
+ * <p>Multi-threaded access is guarded by {@link #mvOperationsLock}.
*/
- private final Set<RowId> rowIdsToSkip = new ConcurrentHashSet<>();
+ private final Set<RowId> rowIdsToSkip = new HashSet<>();
// TODO: IGNITE-18018 - manage queue size
/**
- * Rows that need to be sent out of order (relative to the order in which
this snapshot sends rows).
- * Versions inside rows are in oldest-to-newest order.
+ * Rows that need to be sent out of order (relative to the order in which
this snapshot sends rows). Versions inside rows are in
+ * oldest-to-newest order.
+ *
+ * <p>Multi-threaded access is guarded by {@link #mvOperationsLock}.
*/
private final Queue<SnapshotMvDataResponse.ResponseEntry> outOfOrderMvData
= new ArrayDeque<>();
/**
- * Current delivery state of MV partition data. Can be {@code null} only
if the delivery has not started yet.
+ * Current delivery state of MV partition data.
+ *
+ * <p>Multi-threaded access is guarded by {@link #mvOperationsLock}.
*/
@Nullable
private MvPartitionDeliveryState mvPartitionDeliveryState;
+ /**
+ * Cursor over TX data.
+ *
+ * <p>Inter-thread visibility is provided by accessing {@link
#finishedTxData} in a correct order.
Review Comment:
What does 'in a correct order' mean? Does it mean that happens-before is
established because of network calls causality?
##########
modules/partition-replicator/src/main/java/org/apache/ignite/internal/partition/replicator/raft/snapshot/outgoing/OutgoingSnapshot.java:
##########
@@ -168,30 +183,37 @@ void freezeScopeUnderMvLock() {
acquireMvLock();
try {
- frozenMeta = takeSnapshotMeta();
+ int catalogVersion = catalogService.latestCatalogVersion();
+
+ List<PartitionMvStorageAccess> partitionStorages =
freezePartitionStorages(catalogVersion);
+
+ frozenMeta = takeSnapshotMeta(catalogVersion, partitionStorages);
txDataCursor = txState.getAllTxMeta();
+
+ // Write the flag to publish the TX cursor.
Review Comment:
```suggestion
// Write the flag to publish the TX cursor (in the memory model
sense).
```
--
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
