xiangfu0 commented on code in PR #19028:
URL: https://github.com/apache/pinot/pull/19028#discussion_r3632343603
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/BasePartitionUpsertMetadataManager.java:
##########
@@ -577,15 +592,22 @@ public void replaceSegment(ImmutableSegment segment,
IndexSegment oldSegment) {
}
try {
doReplaceSegment(segment, oldSegment);
- if (!(segment instanceof EmptyIndexSegment)) {
+ if (segment instanceof ImmutableSegmentImpl) {
+ ImmutableSegmentImpl immutableSegment = (ImmutableSegmentImpl) segment;
_trackedSegments.add(segment);
if (_enableSnapshot) {
- _updatedSegmentsSinceLastSnapshot.add(segment);
+ _updatedSegmentsSinceLastSnapshot.add(immutableSegment);
Review Comment:
The updated marker is published before snapshot-file membership. During
reload of a segment that already has an on-disk snapshot, `doTakeSnapshot()`
can observe the new object in `_trackedSegments` and this set while it is still
absent from `_segmentsWithSnapshot`, classify it as having no snapshot, then
fail `tryLock()` while the reload holds the segment lock. The second loop
treats that skip as benign and can create snapshots for other segments while
this segment's stale snapshot remains, violating the disjointness required by
preload. Publish snapshot membership before the updated marker in
add/preload/replace, or re-check membership under the segment lock before using
the no-snapshot path; please add a deterministic interleaving test.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
