[controller] Make controller leader transition timeout configurable - #3005
Open
KaiSernLim wants to merge 1 commit into
Open
[controller] Make controller leader transition timeout configurable#3005KaiSernLim wants to merge 1 commit into
KaiSernLim wants to merge 1 commit into
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
A negative configured timeout can currently escape the existing exception handling and may leave the background transition task running without cancellation.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR makes the controller STANDBY→LEADER transition timeout configurable per cluster via controller.standby.to.leader.transition.timeout.ms (defaulting to 10 minutes), addressing cases where initialization can outlive Helix’s wait and leave stale/partially-initialized leader state behind.
Changes:
- Replaced the previously hard-coded (and test-only override) STANDBY→LEADER transition timeout with a
VeniceControllerClusterConfig-backed value. - Hardened leader transition behavior by failing fast if a connected Helix manager already exists, and improved interrupt handling (restoring interrupt status).
- Added/updated unit tests covering default/override timeout behavior, cancellation/interrupt behavior, reset semantics, and stale-manager re-election behavior.
File summaries
| File | Description |
|---|---|
| services/venice-controller/src/main/java/com/linkedin/venice/controller/VeniceControllerStateModel.java | Uses cluster-configured timeout for STANDBY→LEADER and adjusts interrupt/error handling; tweaks reset ordering. |
| services/venice-controller/src/main/java/com/linkedin/venice/controller/VeniceControllerClusterConfig.java | Adds new cluster config field + getter for standby→leader transition timeout with 10-minute default. |
| internal/venice-common/src/main/java/com/linkedin/venice/ConfigKeys.java | Introduces the new config key constant and documentation comment. |
| services/venice-controller/src/test/java/com/linkedin/venice/controller/TestVeniceControllerStateModel.java | Reworks/extends unit tests for timeout/interrupt/reset/stale-manager transition behavior. |
| services/venice-controller/src/test/java/com/linkedin/venice/controller/TestVeniceControllerClusterConfig.java | Adds tests validating the new config’s default and override. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+218
to
+222
| stateTransitionFuture.get(clusterConfig.getControllerStandbyToLeaderTransitionTimeoutMs(), TimeUnit.MILLISECONDS); | ||
| } catch (InterruptedException e) { | ||
| Thread.currentThread().interrupt(); | ||
| LOGGER.error("Failed to execute the controller state transition from STANDBY to LEADER for {}", clusterName, e); | ||
| if (stateTransitionFuture != null && !stateTransitionFuture.isDone()) { |
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.
Problem Statement
VENG-12819 addresses controller STANDBY-to-LEADER transitions that can outlive Helix's wait and leave an initialized manager behind. The existing five-minute timeout is hard-coded, and retrying leadership with stale manager state could let Helix mark the controller leader without fully initialized cluster resources.
Solution
Make the transition timeout configurable per cluster with
controller.standby.to.leader.transition.timeout.ms, defaulting to 600000 ms (10 minutes). Keep initialization and reset serialized on the existing single worker architecture: timed-out work is interrupted, reset waits for initialization that ignores interruption before cleaning resources and disconnecting the manager, and a stale connected manager rejects re-election until reset. Interrupted transition callers retain their interrupt status.Code changes
controller.standby.to.leader.transition.timeout.ms; default:600000ms (10 minutes).Concurrency-Specific Checks
Both reviewer and PR author verified:
How was this PR tested?
Validation:
./gradlew --no-daemon spotlessCheck :services:venice-controller:test --tests com.linkedin.venice.controller.TestVeniceControllerStateModel --tests com.linkedin.venice.controller.TestVeniceControllerClusterConfiggit diff --checkFocused tests cover the 10-minute default and override, timeout cancellation/interruption, reset with a connected manager and null resources, reset waiting for interruption-ignoring initialization, stale-manager rejection and post-reset re-election, and caller interrupt preservation.
Does this PR introduce any user-facing or breaking changes?
🤖 Generated with GitHub Copilot CLI