Jackie-Jiang commented on code in PR #19028:
URL: https://github.com/apache/pinot/pull/19028#discussion_r3619876045
##########
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:
The wait is effectively bounded for the common cases:
- Segment commit/replacement: during the segment download/build, the tracked
segment is still the mutable one, which is skipped before touching the
segmentLock. The immutable segment object only enters `_trackedSegments` at the
end of the replacement, so the wait is bounded by the upsert metadata swap, not
the download/build.
- Segment removal holds the lock for a bounded metadata cleanup.
The one genuinely long hold is a concurrent segment reload/refresh, which is
rare. The previous behavior wasn't free in that case either: skipping left a
stale snapshot on disk and suppressed the second loop, so all the segments
waiting for their first snapshot got deferred as well, hurting restart/preload.
Delaying the start of consumption in that rare case buys snapshot completeness.
The interrupt escape hatch is exactly the consumer stop path
(`RealtimeSegmentDataManager.stop()` interrupts the consumer thread), so
shutdown/segment stop is not blocked by the wait.
If this shows up delaying consumption in practice, we can switch to
`tryLock(timeout)` with a generous timeout that degrades to the old skip
behavior, but I'd rather not reintroduce the coverage loss for a rare case.
--
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]
