Jackie-Jiang commented on code in PR #19028:
URL: https://github.com/apache/pinot/pull/19028#discussion_r3619874887
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/BasePartitionUpsertMetadataManager.java:
##########
@@ -1013,13 +1026,22 @@ protected void doTakeSnapshot() {
for (ImmutableSegmentImpl segment : segmentsWithoutSnapshot) {
String segmentName = segment.getSegmentName();
Lock segmentLock = tableDataManager.getSegmentLock(segmentName);
+ // Unlike the previous loop, skipping a segment here on lock
contention is benign: no snapshot file is
+ // written for it, so the snapshots kept on disk remain disjoint, and
the segment stays tracked as updated
+ // to be retried in the next snapshot round.
boolean locked = segmentLock.tryLock();
if (!locked) {
_logger.warn("Could not get segmentLock to take snapshot for
segment: {} w/o snapshot, skipping",
segmentName);
continue;
}
try {
+ // The segment can be replaced or removed by another thread after it
was collected in the previous loop.
+ // The replaced segment object no longer owns the segment directory,
so skip persisting its bitmaps.
+ if (!_trackedSegments.contains(segment)) {
Review Comment:
The membership check at collection time doesn't hold at processing time.
`_trackedSegments` is a live concurrent set: between the segment being
collected (first loop) and processed here, a Helix thread can replace
(reload/refresh/re-download) or remove it. A replaced segment object no longer
owns the segment directory, and its bitmaps might have been drained by the
replacement, so persisting them would write stale bitmaps into the directory
now owned by the new segment object.
Re-checking after acquiring the segmentLock is race-free because all the
add/replace/remove paths for immutable segments hold the same per-segment lock
— if the object is still tracked at this point, it remains the live owner for
as long as we hold the lock.
Note this hazard predates this PR: a `tryLock()` succeeding right after a
replacement completes would hit the same stale object.
--
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]
