deepthi912 opened a new pull request, #19024: URL: https://github.com/apache/pinot/pull/19024
## Summary - `doTakeSnapshot()` previously called `segmentLock.tryLock()` non-blocking; briefly-held segmentLocks (typically Helix tasks mid-swap) forced us to skip otherwise-snapshottable segments. - Change both call sites to `tryLock(timeoutMs, MILLISECONDS)` where the timeout is a **server-scoped, cluster-config-live-updatable** value. - Default: `2000` ms. Set to `0` to preserve legacy non-blocking behavior. ## What changed - **New cluster config key**: `pinot.server.upsert.snapshot.segment.lock.timeout.ms` (in `CommonConstants.Server`). - **New singleton holder**: `UpsertSnapshotConfig` — `volatile long` value, exposes `getSegmentLockTimeoutMs()`, implements `PinotClusterConfigChangeListener` for live reload. - **Registration** wired in `BaseServerStarter` alongside `SegmentOperationsThrottlerSet` and the other cluster-config listeners. - **Call sites** in `BasePartitionUpsertMetadataManager.doTakeSnapshot()` snapshot the timeout once per cycle (a mid-cycle PATCH does not cause per-segment divergence). - **Interrupt handling**: preserves the thread's interrupt flag and treats the segment as skipped. ## Historical note (why removing the "non-blocking to avoid deadlock" pattern is safe now) The deleted inline comment referenced a deadlock cycle between the consuming thread's snapshot WLock and Helix task threads waiting for snapshot RLock. The `_snapshotLock` field was **removed from `BasePartitionUpsertMetadataManager` in April 2025**, so that cycle no longer exists. Bounded waiting is safe. ## Rolling upgrade Server-only. No wire-protocol impact. Old servers ignore the new cluster config; new servers pick up the default `2000` ms on start. Rolling upgrades are safe. Operators can PATCH the cluster config to change the value without restart. ## Open items called out by code-reviewer (0 CRITICAL, 8 MAJOR, 7 MINOR) Draft PR — the following are known and I plan to address before marking ready-for-review: ### Definitely add before ready - [ ] **Unit tests for `UpsertSnapshotConfig`**: null/non-numeric/negative/cleared cluster-config paths. - [ ] **Unit test for `doTakeSnapshot()`**: `tryLock` returns false → segment skipped and `isSegmentSkipped` gates the second loop. - [ ] **Release-notes entry** documenting the default behavior change and the new cluster-config key. ### Design trade-offs — open to discussion - **Default value 2000 vs. 0**: Reviewer suggests default-off (0) per Pinot's "new features default to OFF" convention. Left at 2000 in this draft because a live cluster-config PATCH makes opt-in/opt-out equally easy either way, and 2000 matches empirical "1 segment × 1-2 sec" contention pattern. Happy to flip if maintainers prefer. - **Class shape**: `UpsertSnapshotConfig` combines static holder + `PinotClusterConfigChangeListener` singleton. Reviewer suggested routing through `UpsertContext` as an instance dependency instead. Kept the static holder pattern here to match `SegmentOperationsThrottlerSet`; open to refactoring. - **Metric for effective value**: not added; can add a `ServerGauge` if maintainers want visibility for oncall. ## Test plan - [ ] `./mvnw test -pl pinot-segment-local -Dtest=BasePartitionUpsertMetadataManagerTest` - [ ] `./mvnw test -pl pinot-segment-local -Dtest=UpsertSnapshotConfigTest` (to be added) - [ ] Manual: PATCH cluster config on a running server, verify next `doTakeSnapshot()` uses the new value. - [ ] Manual: set to `0`, verify legacy non-blocking behavior is preserved. ## Files - `pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java` — new key + default under `CommonConstants.Server`. - `pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/UpsertSnapshotConfig.java` — **new**. - `pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/BasePartitionUpsertMetadataManager.java` — timeout threaded into both `tryLock` sites; single read per snapshot cycle. - `pinot-server/src/main/java/org/apache/pinot/server/starter/helix/BaseServerStarter.java` — startup init + listener registration. -- 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]
