Auto force-commit partial OFFLINE consuming segments - #19083
Open
Vamsi-klu wants to merge 1 commit into
Open
Conversation
When only some replicas of a consuming segment go OFFLINE after stream blips, queries fan in to the remaining replicas indefinitely. Optionally detect mixed CONSUMING+OFFLINE IdealState and force-commit the partition once the segment is old enough, reusing the existing force-commit path behind a config flag.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #19083 +/- ##
============================================
+ Coverage 65.47% 65.49% +0.02%
+ Complexity 1421 1415 -6
============================================
Files 3426 3426
Lines 217315 217374 +59
Branches 34509 34517 +8
============================================
+ Hits 142283 142369 +86
+ Misses 63513 63491 -22
+ Partials 11519 11514 -5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
Author
|
Ready for review — all required CI checks are green on this PR. Issue: #15897 Could the following folks take a look when convenient? Formal GitHub "Request review" is unavailable from a fork contributor account on Thank you! |
Jackie-Jiang
added
release-notes
Contributor
Contributor
There was a problem hiding this comment.
Pull request overview
Adds an opt-in controller-side repair path for LLC realtime partitions where the latest consuming segment is IN_PROGRESS but IdealState shows a mix of CONSUMING + OFFLINE replicas, by triggering a partition-scoped force-commit (age-gated and tracked at-most-once per segment per controller-leader lifetime) and emitting new controller meters.
Changes:
- Add
controller.realtime.segment.autoForceCommitOnPartialOfflineEnabled(defaultfalse) andcontroller.realtime.segment.autoForceCommitOnPartialOfflineMinAgeMs(default300_000) to gate the new behavior. - Implement partial-OFFLINE auto force-commit logic in
ensureAllPartitionsConsuming, including pruning/bounding of “already requested” tracking. - Add controller meters and unit tests covering success, age-gate skip, all-OFFLINE recreate path, paused-table skip, and validateForceCommit failure handling.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/realtime/PinotLLCRealtimeSegmentManager.java |
Implements auto force-commit on partial OFFLINE consuming replicas (feature-flagged), adds tracking + pruning, and avoids combining with OFFLINE→CONSUMING flip in the same tick. |
pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java |
Introduces new controller periodic-task config keys + accessors with safe defaults (feature off by default). |
pinot-common/src/main/java/org/apache/pinot/common/metrics/ControllerMeter.java |
Adds meters for auto force-commit success/failed/skipped. |
pinot-controller/src/test/java/org/apache/pinot/controller/helix/core/realtime/PinotLLCRealtimeSegmentManagerTest.java |
Adds test coverage for partial-OFFLINE auto force-commit behavior and gating. |
Comments suppressed due to low confidence (5)
pinot-controller/src/test/java/org/apache/pinot/controller/helix/core/realtime/PinotLLCRealtimeSegmentManagerTest.java:743
- This section header Javadoc is duplicated multiple times consecutively. Please remove the duplicates and keep just one copy immediately before the related tests.
/**
* Test cases for the scenario where stream partitions increase, and the validation manager is attempting to create
* segments for new partitions. This test assumes that all other factors remain the same (no error conditions or
* inconsistencies in metadata and ideal state).
*/
pinot-controller/src/test/java/org/apache/pinot/controller/helix/core/realtime/PinotLLCRealtimeSegmentManagerTest.java:896
- There are multiple consecutive copies of the same long Javadoc comment describing the repair scenarios. This makes the file much harder to scan. Collapse this to a single Javadoc block.
/**
* Tests that we can repair all invalid scenarios during segment completion.
*
* Segment completion takes place in 3 steps:
* 1. Update committing segment ZK metadata to status DONE
pinot-controller/src/test/java/org/apache/pinot/controller/helix/core/realtime/PinotLLCRealtimeSegmentManagerTest.java:1878
- Redundant duplicate Javadoc blocks were added before this segment-store upload test section. Remove the duplicates and keep a single section header comment.
/**
* Test cases for fixing LLC segment by uploading to segment store if missing
*/
pinot-controller/src/test/java/org/apache/pinot/controller/helix/core/realtime/PinotLLCRealtimeSegmentManagerTest.java:2017
- Redundant duplicate Javadoc blocks were added before this V2 segment-store upload test section. Remove the duplicates and keep a single section header comment.
/**
* Test cases for fixing LLC segment by uploading to segment store if missing
*/
pinot-controller/src/test/java/org/apache/pinot/controller/helix/core/realtime/PinotLLCRealtimeSegmentManagerTest.java:2388
- This Javadoc is repeated three times consecutively. Keep one copy to document the test, and delete the duplicates.
/**
* Verifies that {@code buildPartitionGroupConsumptionStatusFromZKMetadata} produces the same results as
* {@code getPartitionGroupConsumptionStatusList} for the common case where IdealState and ZK metadata are in sync.
* This validates that the optimization in {@code fetchPartitionGroupIdToSmallestOffset} (reusing the pre-computed
* latestSegmentZKMetadataMap instead of rescanning the entire IdealState) does not change behavior.
*/
Comment on lines
+181
to
+192
| /** | ||
| * Test cases for new table being created, and initial segments setup that follows. | ||
| */ | ||
|
|
||
| /** | ||
| * Test cases for new table being created, and initial segments setup that follows. | ||
| */ | ||
|
|
||
| /** | ||
| * Test cases for new table being created, and initial segments setup that follows. | ||
| */ | ||
|
|
Comment on lines
+2650
to
+2664
| /** | ||
| * Reset consumption start offsets for selected partition groups (issue #6637). | ||
| * For each partition: OFFLINE the latest CONSUMING segment if needed, then create a new IN_PROGRESS | ||
| * consuming segment at the requested offset. Does not attempt to seal/commit poisoned segments. | ||
| * | ||
| * <p>Best-effort per partition: successes and failures are returned independently. Concurrent RSVM/commit | ||
| * races and pauseless edge cases are not fully serialized yet (IdealState is written via {@link #setIdealState}, | ||
| * not Helix CAS — callers must avoid concurrent commits on the same partitions). | ||
| * | ||
| * @param tableNameWithType realtime table name with type | ||
| * @param partitionToOffsetSerialized map of partition group id → serialized {@link StreamPartitionMsgOffset} | ||
| * @param comment optional operator comment for logs (newlines stripped) | ||
| * @return per-partition result maps with keys status/oldSegment/newSegment/offset/message | ||
| */ | ||
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
When a server hits fatal consumption errors it marks its replica OFFLINE in IdealState. RSVM already recreates a consuming segment when all replicas are OFFLINE, but partial OFFLINE (common after transient stream blips) was left alone. Queries then fan in to the remaining CONSUMING replicas indefinitely — imbalance and partial-result risk.
There is a separate flag that only flips OFFLINE→CONSUMING (#11314 path). This PR implements the issue’s requested approach: partition-scoped force-commit when enough progress is likely, behind an opt-in config.
Impact
validateForceCommitAllowed(partial upsert / drop-OOO guards).How
controller.realtime.segment.autoForceCommitOnPartialOfflineEnabled(default false)controller.realtime.segment.autoForceCommitOnPartialOfflineMinAgeMs(default 5 minutes)ensureAllPartitionsConsuming, when IdealState shows mixed CONSUMING+OFFLINE for an IN_PROGRESS segment and age gate passes, call existing partition-scopedforceCommit.Test plan
PinotLLCRealtimeSegmentManagerTest: force-commit when partial OFFLINE + age ok; age gate skip; all-OFFLINE still recreates; paused table skip; validateForceCommit failure skip../mvnw -pl pinot-controller -am -Dtest=PinotLLCRealtimeSegmentManagerTest -Dsurefire.failIfNoSpecifiedTests=false testRelated
fixes: #15897
Reviewers
Suggested: sajjad-moradi (issue author), maintainers of LLC realtime
Config / release notes
New controller configs (default off) — consider
release-noteslabel.Was generative AI tooling used to co-author this PR?
Generated-by: Grok Build (xAI)