deepthi912 commented on code in PR #19028:
URL: https://github.com/apache/pinot/pull/19028#discussion_r3619847417
##########
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:
there is no bounded wait. It can wait as long as a segment
reload/removal/commit/replacement takes. The only escape hatch is thread
interrupt, I am afraid if it could impact on the next consuming segment
--
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]
