deepthi912 opened a new pull request, #19010: URL: https://github.com/apache/pinot/pull/19010
## Summary `pinot.server.consuming.segment.consistency.mode` was introduced in #17503 with three modes: - `RESTRICTED` (current default) — blocks force-commit/reload during the risky consuming-segment transition window for partial-upsert and `dropOutOfOrderRecord`/`outOfOrderRecordColumn` tables. It only *prevents* the risky operation from running; it doesn't resolve anything. - `PROTECTED` — allows force-commit/reload, and reconciles the upsert metadata back to a consistent state if a discrepancy is detected during segment commit or removal. - `UNSAFE` — allows force-commit/reload with no reconciliation. This PR proposes flipping the default from `RESTRICTED` to `PROTECTED`, since `RESTRICTED` alone doesn't get an affected table to a correct state — it just blocks operational flows (reload, force-commit, Segment Refresh Task, index changes) on partial-upsert / `dropOutOfOrderRecord` tables indefinitely, until an operator manually opts into `PROTECTED`. Defaulting to `PROTECTED` means these operations work out of the box, with the metadata-reconciliation safety net already in place. ## Compatibility An operator who has explicitly set this cluster config (to any of the three modes) is unaffected: `ConsumingSegmentConsistencyModeListener.onChange` only updates the mode when the config key is actually present in the cluster config snapshot passed by `PinotClusterConfigChangeListener` — an absent key never touches this default, and an explicit value always wins, on every config change including the initial sync at registration. ## Known caveat — please weigh in #17503's description calls out: *"During Ingestion, always make sure the `ParallelSegmentConsumptionPolicy` is `DISALLOW_ALWAYS` or `ALLOW_DURING_BUILD_ONLY` for the Partial Upsert and Upsert Tables with `dropOutOfOrder=true` when `consistencyMode=NONE` to avoid any inconsistencies in `PROTECTED` mode. Otherwise, we might see inconsistent records across servers if at any given point we have two segments in Mutable state."* This means `PROTECTED` is not unconditionally safer than `RESTRICTED` for every possible cluster configuration — it's possible this precondition is exactly why `RESTRICTED` was chosen as the conservative default in the first place. I'm opening this PR to start that discussion rather than asserting `PROTECTED` is strictly better in all cases; happy to close or rework based on maintainer feedback (e.g. only flipping the default once `ParallelSegmentConsumptionPolicy` is itself validated/defaulted safely, or documenting the precondition more prominently instead). ## Changes - `ConsumingSegmentConsistencyModeListener`: default flipped to `PROTECTED`, class Javadoc updated to match. - New `ConsumingSegmentConsistencyModeListenerTest`: covers the new default, explicit cluster-config override (including that an unrelated changed key doesn't touch the mode), missing/blank/invalid value fallback, and `reset()`. - `PartialUpsertTableRebalanceIntegrationTest`: updated a comment that assumed `RESTRICTED` was still the default (the test already sets `PROTECTED` explicitly, so behavior is unchanged). ## Test plan - [x] `./mvnw spotless:apply / license:format / checkstyle:check / license:check -pl pinot-spi,pinot-integration-tests` all pass - [x] `./mvnw -pl pinot-spi -Dtest=ConsumingSegmentConsistencyModeListenerTest test` — 6/6 pass -- 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]
