Jackie-Jiang opened a new pull request, #19028: URL: https://github.com/apache/pinot/pull/19028
## Summary `BasePartitionUpsertMetadataManager.doTakeSnapshot()` used `tryLock()` on the segmentLock and skipped the segment on contention. The non-blocking acquire was introduced in #14179 to avoid a deadlock with Helix task threads: back then the consuming thread held the snapshot WLock while waiting for the segmentLock, and Helix task threads acquired the segmentLock first and then the snapshot RLock. Since #15420 removed the snapshot RW lock, the snapshot thread no longer holds any lock that segmentLock holders can wait on, so the deadlock cycle no longer exists and blocking on the segmentLock is safe. This PR changes the first loop (segments with existing snapshot files) to a blocking acquire, so transient lock contention no longer skips the snapshot round and no longer suppresses the second loop (first-time snapshots), improving snapshot coverage and restart/preload behavior. Contention should be rare: the most common segment replacement is for the just committed consuming segment, but the tracked segment remains the mutable one (skipped by the snapshot loop) until the immutable segment is swapped in at the end of the replacement, so the wait is bounded by the upsert metadata replacement instead of the segment download/build. Details: - Use `lockInterruptibly()` instead of `lock()`: `takeSnapshot()` runs on the consumer thread, and `RealtimeSegmentDataManager.stop()` terminates that thread with an interrupt + join loop, potentially from a Helix thread already holding a segmentLock. An uninterruptible acquire would make `stop()` wait until the contended lock frees (e.g. a concurrent segment reload). This matches the existing `lockInterruptibly()` usage on segmentLocks in `RealtimeSegmentDataManager`. On interrupt, the snapshot round is aborted and the second loop is suppressed. - Re-check `_trackedSegments.contains(segment)` after acquiring the segmentLock: the segment can be replaced (reload/refresh/re-download) or removed while waiting, and the dead segment object no longer owns the segment directory, so persisting its bitmaps could write stale content into the directory now owned by the new segment object. The same re-check is added to the second loop, where a `tryLock()` succeeding right after a replacement completes could hit the same stale object. - The second loop keeps `tryLock()` + skip, now documented: skipping a first-time snapshot writes no file, so the snapshots kept on disk remain disjoint, and the segment stays tracked as updated to be retried in the next round. - Cleaned up stale comments still referencing the snapshot RW lock removed in #15420. - Rewrote the race-condition tests, which pinned the skip-on-contention behavior (and would deadlock against a blocking acquire), to assert the new contract: the snapshot thread blocks on a held segmentLock, persists nothing while waiting, and takes all snapshots once the lock is released. Added tests for the interrupt-abort path and for the retained skip in the second loop. -- 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]
