Jackie-Jiang commented on code in PR #19028:
URL: https://github.com/apache/pinot/pull/19028#discussion_r3626253366
##########
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:
Update: the check is kept in the reworked design, in both loops. To clarify
the two complementary windows it covers together with `tryLock()`:
- A failed `tryLock()` catches a segment operation in progress (the other
thread still holds the segmentLock).
- This re-check catches an operation that completed between classification
and lock acquisition: the replacement finished and released the lock, so
`tryLock()` succeeds on the dead segment object. The window is wider now that
segments are classified up front and persisted later.
The re-check is race-free because all add/replace/remove paths for immutable
segments hold the same per-segment lock, so a segment that is still tracked
under the lock remains the live owner of the segment directory until the lock
is released.
--
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]
