Jackie-Jiang commented on code in PR #19028:
URL: https://github.com/apache/pinot/pull/19028#discussion_r3626254473
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/BasePartitionUpsertMetadataManager.java:
##########
@@ -951,27 +952,39 @@ protected void doTakeSnapshot() {
numUnchangedSegments++;
continue;
}
- // Try to acquire the segmentLock when taking snapshot for the segment
because the segment directory can be
- // modified, e.g. a new snapshot file can be added to the directory. If
not taking the lock, the Helix task
- // thread replacing the segment could fail. For example, we found
FileUtils.cleanDirectory() failed due to
+ // Acquire the segmentLock when taking snapshot for the segment because
the segment directory can be modified,
+ // e.g. a new snapshot file can be added to the directory. If not taking
the lock, the Helix task thread
+ // replacing the segment could fail. For example, we found
FileUtils.cleanDirectory() failed due to
// DirectoryNotEmptyException because a new snapshot file got added into
the segment directory just between two
// major cleanup steps in the cleanDirectory() method.
+ // Blocking on the segmentLock cannot deadlock because this thread holds
no lock that segmentLock holders can
+ // wait on. Contention should also be rare: the most common segment
replacement is for the just committed
+ // consuming segment, but the tracked segment remains the mutable one
(thus skipped above) until the immutable
+ // segment is swapped in at the end of the replacement, so the wait is
bounded by the metadata replacement
+ // instead of the segment download/build. A concurrent segment
reload/refresh can hold the segmentLock for its
+ // full duration though, which delays the snapshot and the start of
consumption.
+ // Lock interruptibly so that stopping the consumer thread is not
blocked by a long-held segmentLock.
String segmentName = segment.getSegmentName();
Lock segmentLock = tableDataManager.getSegmentLock(segmentName);
- boolean locked = segmentLock.tryLock();
- if (!locked) {
- // Try to get the segmentLock in a non-blocking manner to avoid
deadlock. The Helix task thread takes
- // segmentLock first and then the snapshot RLock when replacing a
segment. However, the consuming thread has
- // already acquired the snapshot WLock when reaching here, and if it
has to wait for segmentLock, it may
- // enter deadlock with the Helix task threads waiting for snapshot
RLock.
- // If we can't get the segmentLock, we'd better skip taking snapshot
for this tracked segment, because its
- // validDocIds/queryableDocIds might become stale or wrong when the
segment is being processed by another
- // thread right now.
- _logger.warn("Could not get segmentLock to take snapshot for segment:
{}, skipping", segmentName);
+ try {
+ segmentLock.lockInterruptibly();
Review Comment:
Good point — reworked the PR to address this. The blocking wait is gone:
both loops use `tryLock()` again, so there is no unbounded wait on the
consuming thread.
The real problem with the old skip behavior was that the most common
contention — the just committed segment still being replaced with the immutable
one — gated the second loop and deferred every other segment's first snapshot.
That segment has no snapshot file yet, so skipping it alone is benign for the
on-disk disjointness. The new design tracks the segments with on-disk snapshots
in `_segmentsWithSnapshot`, so segments are classified without touching the
segmentLock: a contended segment without a snapshot file is now skipped by
itself (retried in the next round), while a contended segment with an existing
snapshot still gates the creation of new snapshot files to keep the snapshots
on disk disjoint. As a bonus, each segmentLock is acquired only once per
snapshot round.
--
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]
