Ability to trigger auto rebalance on imbalance feature - #13037
Ability to trigger auto rebalance on imbalance feature #13037ShubhamRwt wants to merge 3 commits into
Conversation
Signed-off-by: ShubhamRwt <[email protected]>
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
Signed-off-by: ShubhamRwt <[email protected]>
|
@ShubhamRwt can we get a green build before starting for an early review please? Failure are around the CruiseControlReconciler so maybe something to fix in the logic before taking a look. Thanks :-) |
Signed-off-by: ShubhamRwt <[email protected]>
|
@ppatierno All the tests seems to be passing now |
ppatierno
left a comment
There was a problem hiding this comment.
@ShubhamRwt I had a first quick pass but it needs more detailed review.
| "been changed to match the specified default goals."); | ||
| // If the user has not explicitly configured anomaly detection goals, default them to the default anomaly detection goals | ||
| if (configuration.getConfigOption(CruiseControlConfigurationParameters.ANOMALY_DETECTION_CONFIG_KEY.toString()) == null) { | ||
| configuration.setConfigOption(CruiseControlConfigurationParameters.ANOMALY_DETECTION_CONFIG_KEY.toString(), CRUISE_CONTROL_DEFAULT_ANOMALY_DETECTION_GOALS); |
There was a problem hiding this comment.
why the previous logic changed so heavily. Weren't we checking that anomaly detection goals should be in the default goals as well otherwise CC fails on startup? I see this logic now removed.
| private Map<String, ResourceRequirements> kafkaBrokerResources; | ||
| // needed to take information for the auto-rebalancing on scaling via Cruise Control | ||
| private Set<Integer> scalingDownBlockedNodes; | ||
| Set<Integer> scalingDownBlockedNodes; |
| * @return Future with boolean indicating if rebalance should be triggered | ||
| */ | ||
| public CompletionStage<Boolean> shouldTriggerRebalance(Instant detectionDate) { | ||
| String configMapName = reconciliation.name() + KafkaAutoRebalancingReconciler.AUTO_REBALANCE_IMBALANCE_TRACKER_SUFFIX; |
There was a problem hiding this comment.
we can avoid this additional variable.
| "RackAwareGoal", | ||
| "MinTopicLeadersPerBrokerGoal", | ||
| "ReplicaCapacityGoal", | ||
| "DiskCapacityGoal" |
There was a problem hiding this comment.
why do we have these hard-coded here, should we have them somewhere in the Cruise Control api classes, IIRC we have some default constants there?
There was a problem hiding this comment.
I was referring to CruiseControlConfiguration.CRUISE_CONTROL_DEFAULT_ANOMALY_DETECTION_GOALS
| .onComplete(v -> kafkaStatus.setAutoRebalance(kafkaAutoRebalanceStatus)); | ||
| } | ||
|
|
||
| return Future.<Boolean>fromCompletionStage(imbalanceDetector.validateTemplateGoals()) |
There was a problem hiding this comment.
I see this pattern of using fromCompletionStage motsly everywhere when you have calls coming from the detector. I think it means that all detector methods are badly declared returning a CompletionStage while they should return a CompletableFuture instead.
| return kafkaRebalance.getStatus(); | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
remove this blank line, so this file won't change :-)
| .withParameter(CruiseControlParameters.JSON, "true") | ||
| .withParameter(CruiseControlParameters.FETCH_COMPLETE, "true"); | ||
| .withParameter(CruiseControlParameters.JSON, "true") | ||
| .withParameter(CruiseControlParameters.FETCH_COMPLETE, "true"); |
There was a problem hiding this comment.
why this change? Even for the others below, why changing the indentation? I think something different in your IDE?
There was a problem hiding this comment.
Yeah, I have some formatting filter in my IDE, I will fix these
| } else if (ex.getCause() instanceof RuntimeException) { | ||
| return (RuntimeException) ex.getCause(); | ||
| } else { | ||
| // Wrap checked exceptions (like IOException) in RuntimeException |
There was a problem hiding this comment.
the comment says wrapping into RuntimeException but the code is wrapping into CruiseControlRetriableConnectionException.
| * | ||
| * @return Future which completes when the check is done | ||
| */ | ||
| private Future<Void> maybeCheckForImbalance(ScalingNodes scalingNodes, KafkaStatus kafkaStatus) { |
There was a problem hiding this comment.
I don't get why the check for imbalance method needs the scalingNodes. I think this could be handled outside of it within the reconcile? I don't expect this method to call maybeRebalance internally by providing the scalingNodes. I don't have a solution right now to provide but maybe we could re-arrange the code to be more understandable.
| "RackAwareGoal", | ||
| "MinTopicLeadersPerBrokerGoal", | ||
| "ReplicaCapacityGoal", | ||
| "DiskCapacityGoal" |
There was a problem hiding this comment.
I was referring to CruiseControlConfiguration.CRUISE_CONTROL_DEFAULT_ANOMALY_DETECTION_GOALS
Type of change
Description
This PR implements the proposal auto-rebalance-on-imbalance. This feature allows the operator to trigger rebalance automatically in case their are some imbalance in the cluster
Checklist
Please go through this checklist and make sure all applicable tasks have been done