-
Notifications
You must be signed in to change notification settings - Fork 604
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add endpoint to move all replicas from a disk to other disks of the same broker #1908
Changes from all commits
7167692
4e20c47
ab739e1
b1df366
ad27507
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -43,19 +43,31 @@ public class IntraBrokerDiskCapacityGoal extends AbstractGoal { | |
private static final Logger LOG = LoggerFactory.getLogger(IntraBrokerDiskCapacityGoal.class); | ||
private static final int MIN_NUM_VALID_WINDOWS = 1; | ||
private static final Resource RESOURCE = Resource.DISK; | ||
// This is only used for the remove disks endpoint | ||
private final boolean _shouldEmptyZeroCapacityDisks; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also please add a comment on this variable, something like "This is only used for removeReplicasFromBroker endpoint" There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good note. Added the comment in the latest commit. |
||
|
||
/** | ||
* Constructor for Capacity Goal. | ||
*/ | ||
public IntraBrokerDiskCapacityGoal() { | ||
_shouldEmptyZeroCapacityDisks = false; | ||
} | ||
|
||
/** | ||
* Constructor for Intra Broker Disk Capacity Goal. | ||
* | ||
* @param shouldEmptyZeroCapacityDisks specifies if the goal should move all replicas from disks with 0 capacity | ||
*/ | ||
public IntraBrokerDiskCapacityGoal(boolean shouldEmptyZeroCapacityDisks) { | ||
_shouldEmptyZeroCapacityDisks = shouldEmptyZeroCapacityDisks; | ||
} | ||
|
||
/** | ||
* Package private for unit test. | ||
*/ | ||
IntraBrokerDiskCapacityGoal(BalancingConstraint constraint) { | ||
_balancingConstraint = constraint; | ||
_shouldEmptyZeroCapacityDisks = false; | ||
} | ||
|
||
@Override | ||
|
@@ -149,8 +161,15 @@ public ActionAcceptance actionAcceptance(BalancingAction action, ClusterModel cl | |
protected boolean selfSatisfied(ClusterModel clusterModel, BalancingAction action) { | ||
Replica sourceReplica = clusterModel.broker(action.sourceBrokerId()).replica(action.topicPartition()); | ||
Disk destinationDisk = clusterModel.broker(action.destinationBrokerId()).disk(action.destinationBrokerLogdir()); | ||
return sourceReplica.load().expectedUtilizationFor(RESOURCE) > 0 | ||
&& isMovementAcceptableForCapacity(sourceReplica, destinationDisk); | ||
boolean shouldMoveReplica; | ||
if (_shouldEmptyZeroCapacityDisks) { | ||
// should also move replicas with 0 expected disk utilization | ||
shouldMoveReplica = sourceReplica.load().expectedUtilizationFor(RESOURCE) >= 0; | ||
} else { | ||
// In general CC tries to reduce number of replica move. Since empty replicas does not affect balance, CC tries to avoid moving them. | ||
shouldMoveReplica = sourceReplica.load().expectedUtilizationFor(RESOURCE) > 0; | ||
} | ||
return shouldMoveReplica && isMovementAcceptableForCapacity(sourceReplica, destinationDisk); | ||
} | ||
|
||
/** | ||
|
@@ -221,6 +240,10 @@ protected void updateGoalState(ClusterModel clusterModel, OptimizationOptions op | |
throw new OptimizationFailureException(String.format("[%s] Utilization (%.2f) for disk %s on broker %d is above capacity limit.", | ||
name(), disk.utilization(), disk, broker.id()), recommendation); | ||
} | ||
if (_shouldEmptyZeroCapacityDisks && disk.capacity() == 0 && !disk.replicas().isEmpty()) { | ||
throw new OptimizationFailureException(String.format("[%s] Could not move all replicas from disk %s on broker %d", | ||
name(), disk.logDir(), broker.id())); | ||
} | ||
} | ||
} | ||
finish(); | ||
|
@@ -239,11 +262,15 @@ public ModelCompletenessRequirements clusterModelCompletenessRequirements() { | |
|
||
/** | ||
* Check whether the combined replica utilization is above the given disk capacity limits. | ||
* If _shouldEmptyZeroCapacityDisks is true, the disk utilization is over limit only if it is greater than 0. | ||
* | ||
* @param disk Disk to be checked for capacity limit violation. | ||
* @return {@code true} if utilization is over the limit, {@code false} otherwise. | ||
*/ | ||
private boolean isUtilizationOverLimit(Disk disk) { | ||
if (_shouldEmptyZeroCapacityDisks && disk.capacity() == 0) { | ||
return disk.utilization() > 0 || !disk.replicas().isEmpty(); | ||
} | ||
return disk.utilization() > disk.capacity() * _balancingConstraint.capacityThreshold(RESOURCE); | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,46 @@ | ||
/* | ||
* Copyright 2022 LinkedIn Corp. Licensed under the BSD 2-Clause License (the "License"). See License in the project root for license information. | ||
*/ | ||
|
||
package com.linkedin.kafka.cruisecontrol.servlet.handler.async; | ||
|
||
import com.linkedin.kafka.cruisecontrol.servlet.handler.async.runnable.OperationFuture; | ||
import com.linkedin.kafka.cruisecontrol.servlet.handler.async.runnable.RemoveDisksRunnable; | ||
import com.linkedin.kafka.cruisecontrol.servlet.parameters.RemoveDisksParameters; | ||
import java.util.Map; | ||
|
||
import static com.linkedin.cruisecontrol.common.utils.Utils.validateNotNull; | ||
import static com.linkedin.kafka.cruisecontrol.servlet.parameters.ParameterUtils.REMOVE_DISKS_PARAMETER_OBJECT_CONFIG; | ||
|
||
public class RemoveDisksRequest extends AbstractAsyncRequest { | ||
protected RemoveDisksParameters _parameters; | ||
|
||
public RemoveDisksRequest() { | ||
super(); | ||
} | ||
|
||
@Override | ||
protected OperationFuture handle(String uuid) { | ||
OperationFuture future = new OperationFuture("Remove disks"); | ||
pending(future.operationProgress()); | ||
_asyncKafkaCruiseControl.sessionExecutor().submit(new RemoveDisksRunnable(_asyncKafkaCruiseControl, future, _parameters, uuid)); | ||
return future; | ||
} | ||
|
||
@Override | ||
public RemoveDisksParameters parameters() { | ||
return _parameters; | ||
} | ||
|
||
@Override | ||
public String name() { | ||
return RemoveDisksRequest.class.getSimpleName(); | ||
} | ||
|
||
@Override | ||
public void configure(Map<String, ?> configs) { | ||
super.configure(configs); | ||
_parameters = (RemoveDisksParameters) validateNotNull(configs.get(REMOVE_DISKS_PARAMETER_OBJECT_CONFIG), | ||
"Parameter configuration is missing from the request."); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious. Even if we don't add this boolean, shouldn't cruise control move replicas out if you set disk capacity to 0?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be more intuitive if cruise control moved all replicas if the disk capacity is set to 0. However, initial tests showed that it does not (in the case of 0 load replicas) and I added this flag to make sure I do not change how the goal worked before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only reason I can think of is cc tries to avoid moving replicas if possible. Since empty replicas does not affect balance, CC tires to avoid it.