Fix throttle removal verification hang when a default replication throttle is set - #2385
Open
acgtun wants to merge 1 commit into
Open
Fix throttle removal verification hang when a default replication throttle is set#2385acgtun wants to merge 1 commit into
acgtun wants to merge 1 commit into
Conversation
…ottle is set When clearing throttles, ReplicationThrottleHelper deletes the per-broker leader/follower.replication.throttled.rate configs and then polls describeConfigs until the configs read back as absent. If a cluster-wide dynamic default throttle exists (set via kafka-configs --alter --entity-type brokers --entity-default), the deleted per-broker config resolves to the default value, so describeConfigs keeps returning a non-null entry and verification never converges. configsEqual already skipped this comparison for STATIC_BROKER_CONFIG; extend the same treatment to DYNAMIC_DEFAULT_BROKER_CONFIG and DEFAULT_CONFIG: seeing a value from one of these sources after a DELETE means the per-entity delete itself succeeded and the remaining value is inherited from a layer Cruise Control does not manage. Also cap the verification retry backoff at 30s per attempt. The retry helper doubles the sleep with no cap (5s * 2^n), so a verification that keeps failing parks the executor thread in exponentially growing sleeps (attempt 13 alone sleeps ~11 hours) and the whole execution appears hung. With the cap, retries exhaust in ~15 minutes and fail loudly. Observed in production on a 200+ broker cluster: the executor completed its first batch of movements, then slept for hours inside clearThrottles verification because an --entity-default throttle was present; the rebalance made no further progress and new executions were rejected with "Cannot start a new execution".
Author
|
Updated the description to follow the PR template format. |
There was a problem hiding this comment.
Pull request overview
This PR fixes a throttle-removal verification hang in ReplicationThrottleHelper when brokers inherit replication throttle values from cluster-wide defaults (e.g., kafka-configs --entity-default), and bounds the verification retry backoff so a stuck verification fails within a reasonable time instead of sleeping exponentially for hours/days.
Changes:
- Treat config entries inherited from
STATIC_BROKER_CONFIG,DYNAMIC_DEFAULT_BROKER_CONFIG, andDEFAULT_CONFIGas “delete applied” when the expected per-entity value isnull, allowing delete verification to converge under default layers. - Cap
waitForConfigsverification retry backoff at 30s per attempt by using theCruiseControlMetricsUtils.retry(..., maxSleepMs)overload. - Extend unit tests for
configsEqualto cover dynamic-default and Kafka-default config sources.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| cruise-control/src/main/java/com/linkedin/kafka/cruisecontrol/executor/ReplicationThrottleHelper.java | Updates config verification semantics for inherited defaults after DELETE and caps retry backoff during verification. |
| cruise-control/src/test/java/com/linkedin/kafka/cruisecontrol/executor/ReplicationThrottleHelperTest.java | Adds configsEqual test cases for DYNAMIC_DEFAULT_BROKER_CONFIG and DEFAULT_CONFIG behaviors. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
381
to
+385
| return !configsEqual(getEntityConfigs(cf), expectedConfigs); | ||
| } catch (ExecutionException | InterruptedException | TimeoutException e) { | ||
| return false; | ||
| } | ||
| }, _retries); | ||
| }, RETRY_BACKOFF_SCALE_MS, RETRY_BACKOFF_BASE, _retries, MAX_RETRY_SLEEP_MS); |
Comment on lines
+45
to
+47
| // Backoff parameters for the config verification retry loop. The sleep between attempts is | ||
| // scale * base^attempt, capped at MAX_RETRY_SLEEP_MS so that a slow-to-verify config cannot | ||
| // park the executor thread for an unbounded exponential sleep. |
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.
Summary
ReplicationThrottleHelperdeletes the per-brokerleader/follower.replication.throttled.rateconfigs and then pollsdescribeConfigsuntil the configs read back as absent. If a cluster-wide dynamic default throttle exists (set viakafka-configs --alter --entity-type brokers --entity-default), the deleted per-broker config resolves to the default value, sodescribeConfigskeeps returning a non-null entry,configsEqualkeeps returning false, and verification never converges. Worse, the retry helper (CruiseControlMetricsUtils.retry) doubles the sleep between attempts with no cap (5s * 2^n), so the executor thread ends up parked in exponentially growing sleeps — attempt 13 alone sleeps ~11 hours; exhausting 30 retries would take centuries.configsEqualalready skipped the comparison when a deleted config resolves toSTATIC_BROKER_CONFIG; extend the same treatment toDYNAMIC_DEFAULT_BROKER_CONFIGandDEFAULT_CONFIG— seeing a value from one of these sources after a DELETE means the per-entity delete itself succeeded and the remaining value is inherited from a layer Cruise Control does not manage. (b) Cap the verification retry backoff at 30s per attempt, so a genuinely failing verification exhausts its 30 retries in ~15 minutes and fails loudly with the existingIllegalStateExceptioninstead of parking the executor for hours/days.Expected Behavior
After a movement batch completes, throttle removal verification converges and the execution proceeds to the next batch, regardless of whether a cluster-wide default replication throttle is set.
Actual Behavior
Throttle removal verification never converges when an
--entity-defaultreplication throttle exists. The executor thread parks in exponentially growing sleeps insidewaitForConfigs, the execution appears hung (no further movements dispatched), and new executions are rejected withCannot start a new execution while there is an ongoing execution.Steps to Reproduce
kafka-configs --alter --entity-type brokers --entity-default --add-config leader.replication.throttled.rate=...,follower.replication.throttled.rate=...rebalance?dryrun=false).clearThrottlesdeletes the per-broker rates, and verification loops forever because the deleted configs resolve to the dynamic default layer.Known Workarounds
Remove the
--entity-defaultreplication throttle before running an execution, or restart Cruise Control to abort the stuck execution.Additional evidence
--entity-defaultlayer:testConfigsEqualwith cases forDYNAMIC_DEFAULT_BROKER_CONFIGandDEFAULT_CONFIGsources (deleted configs treated as applied; non-null expected values still compared); fullReplicationThrottleHelperTestsuite (unit + embedded-Kafka integration) passes; checkstyle clean.Categorization