Skip to content
Merged
Original file line number Diff line number Diff line change
Expand Up @@ -312,7 +312,7 @@ Map<String, Pair<Boolean, String>> getSafeModeRuleStatuses()
* Start ContainerBalancer.
*/
boolean startContainerBalancer(Optional<Double> threshold,
Optional<Integer> idleiterations,
Optional<Integer> iterations,
Optional<Double> maxDatanodesRatioToInvolvePerIteration,
Optional<Long> maxSizeToMovePerIterationInGB,
Optional<Long> maxSizeEnteringTargetInGB,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -312,7 +312,7 @@ Map<String, Pair<Boolean, String>> getSafeModeRuleStatuses()
* Start ContainerBalancer.
*/
boolean startContainerBalancer(Optional<Double> threshold,
Optional<Integer> idleiterations,
Optional<Integer> iterations,
Optional<Double> maxDatanodesRatioToInvolvePerIteration,
Optional<Long> maxSizeToMovePerIterationInGB,
Optional<Long> maxSizeEnteringTargetInGB,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -737,7 +737,7 @@ public boolean getReplicationManagerStatus() throws IOException {

@Override
public boolean startContainerBalancer(
Optional<Double> threshold, Optional<Integer> idleiterations,
Optional<Double> threshold, Optional<Integer> iterations,
Optional<Double> maxDatanodesRatioToInvolvePerIteration,
Optional<Long> maxSizeToMovePerIterationInGB,
Optional<Long> maxSizeEnteringTargetInGB,
Expand Down Expand Up @@ -769,12 +769,12 @@ public boolean startContainerBalancer(
"lesser than equal to one.");
builder.setMaxDatanodesRatioToInvolvePerIteration(mdti);
}
if (idleiterations.isPresent()) {
int idi = idleiterations.get();
Preconditions.checkState(idi > 0 || idi == -1,
"idleiterations must be positive or" +
if (iterations.isPresent()) {
int i = iterations.get();
Preconditions.checkState(i > 0 || i == -1,
"iterations must be positive or" +
" -1(infinitly run container balancer).");
builder.setIdleiterations(idi);
builder.setIterations(i);
}

if (maxSizeEnteringTargetInGB.isPresent()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -479,7 +479,7 @@ message GetContainerTokenResponseProto {
message StartContainerBalancerRequestProto {
optional string traceID = 1;
optional double threshold = 2;
optional int32 idleiterations = 3;
optional int32 iterations = 3;
optional double maxDatanodesRatioToInvolvePerIteration = 4;
optional int64 maxSizeToMovePerIterationInGB = 5;
optional int64 maxSizeEnteringTargetInGB = 6;
Expand Down
2 changes: 1 addition & 1 deletion hadoop-hdds/interface-admin/src/main/resources/proto.lock
Original file line number Diff line number Diff line change
Expand Up @@ -1334,7 +1334,7 @@
},
{
"id": 3,
"name": "idleiterations",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't EVER change proto lock files! These are automatically updated by protolock only on each major or minor release to ensure compatibility. This is an easy mistake to make when proto-backwards-compatability-check fails though, and it would definitely be helpful if our CI could help the devs out here : ) @adoroszlai is there an easy check we can put in CI to fail PRs that that modify anything called proto.lock?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. This seems to have sneaked in while I was refactoring this field's name across the project. CI got skipped since this is still a draft PR.

BTW, from what I understand, it's compatible to change the name of a proto field as long as the ID and type remain the same, and JSON is not involved. The hadoop policy that's linked here also says it's compatible.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah the rename in the proto itself is safe, lets just make sure the proto lock stays in its original state.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing only the proto file fails the backwards compatibility check here:

[INFO] CONFLICT: "StartContainerBalancerRequestProto" field: "idleiterations" has been removed, but is not reserved [ScmAdminProtocol.proto]
[INFO] CONFLICT: "StartContainerBalancerRequestProto" field: "iterations" ID: 3 has an updated name, previously "idleiterations" [ScmAdminProtocol.proto]

With the proto lock file in its original state.

"name": "iterations",
"type": "int32"
},
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ public class ContainerBalancer {
private long maxSizeToMovePerIteration;
private int countDatanodesInvolvedPerIteration;
private long sizeMovedPerIteration;
private int idleIteration;
private int iterations;
private List<DatanodeUsageInfo> unBalancedNodes;
private List<DatanodeUsageInfo> overUtilizedNodes;
private List<DatanodeUsageInfo> underUtilizedNodes;
Expand Down Expand Up @@ -170,16 +170,16 @@ public boolean start(ContainerBalancerConfiguration balancerConfiguration) {
* Balances the cluster.
*/
private void balance() {
this.idleIteration = config.getIdleIteration();
if(this.idleIteration == -1) {
this.iterations = config.getIterations();
if(this.iterations == -1) {
//run balancer infinitely
this.idleIteration = Integer.MAX_VALUE;
this.iterations = Integer.MAX_VALUE;
}
this.threshold = config.getThreshold();
this.maxDatanodesRatioToInvolvePerIteration =
config.getMaxDatanodesRatioToInvolvePerIteration();
this.maxSizeToMovePerIteration = config.getMaxSizeToMovePerIteration();
for (int i = 0; i < idleIteration && balancerRunning; i++) {
for (int i = 0; i < iterations && balancerRunning; i++) {
// stop balancing if iteration is not initialized
if (!initializeIteration()) {
stop();
Expand All @@ -202,7 +202,7 @@ private void balance() {

// wait for configured time before starting next iteration, unless
// this was the final iteration
if (i != idleIteration - 1) {
if (i != iterations - 1) {
synchronized (this) {
try {
wait(config.getBalancingInterval().toMillis());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,22 +76,23 @@ public final class ContainerBalancerConfiguration {
defaultValue = "", tags = {ConfigTag.BALANCER}, description = "The " +
"maximum size that can enter a target datanode in each " +
"iteration while balancing. This is the sum of data from multiple " +
"sources. The default value is greater than the configured" +
" (or default) ozone.scm.container.size by 1GB.")
"sources. By default, five times the configured (or default) ozone.scm" +
".container.size is allowed.")
private long maxSizeEnteringTarget;

@Config(key = "size.leaving.source.max", type = ConfigType.SIZE,
defaultValue = "", tags = {ConfigTag.BALANCER}, description = "The " +
"maximum size that can leave a source datanode in each " +
"iteration while balancing. This is the sum of data moving to multiple " +
"targets. The default value is greater than the configured" +
" (or default) ozone.scm.container.size by 1GB.")
"targets. By default, five times the configured (or default) ozone.scm" +
".container.size is allowed.")
private long maxSizeLeavingSource;

@Config(key = "idle.iterations", type = ConfigType.INT,
@Config(key = "iterations", type = ConfigType.INT,
defaultValue = "10", tags = {ConfigTag.BALANCER},
description = "The idle iteration count of Container Balancer.")
private int idleIterations = 10;
description = "The number of iterations that Container Balancer will " +
"run for.")
private int iterations = 10;

@Config(key = "exclude.containers", type = ConfigType.STRING, defaultValue =
"", tags = {ConfigTag.BALANCER}, description = "List of container IDs " +
Expand Down Expand Up @@ -136,14 +137,13 @@ public ContainerBalancerConfiguration(OzoneConfiguration config) {
"OzoneConfiguration should not be null.");
this.ozoneConfiguration = config;

// maxSizeEnteringTarget and maxSizeLeavingSource should by default be
// greater than container size
long size = (long) ozoneConfiguration.getStorageSize(
// maxSizeEnteringTarget and maxSizeLeavingSource should by default allow
// five containers
long size = 5 * (long) ozoneConfiguration.getStorageSize(
ScmConfigKeys.OZONE_SCM_CONTAINER_SIZE,
ScmConfigKeys.OZONE_SCM_CONTAINER_SIZE_DEFAULT, StorageUnit.GB) +
OzoneConsts.GB;
maxSizeEnteringTarget = size;
maxSizeLeavingSource = size;
ScmConfigKeys.OZONE_SCM_CONTAINER_SIZE_DEFAULT, StorageUnit.BYTES);
maxSizeEnteringTarget = size + OzoneConsts.GB;
maxSizeLeavingSource = size + OzoneConsts.GB;

// balancing interval should be greater than DUFactory refresh period
duConf = ozoneConfiguration.getObject(DUFactory.Conf.class);
Expand Down Expand Up @@ -174,26 +174,27 @@ public void setThreshold(double threshold) {
}

/**
* Gets the idle iteration value for Container Balancer.
* Gets the iteration count for Container Balancer. A value of -1 means
* infinite number of iterations.
*
* @return a idle iteration count larger than 0
* @return a value greater than 0, or -1
*/
public int getIdleIteration() {
return idleIterations;
public int getIterations() {
return iterations;
}

/**
* Sets the idle iteration value for Container Balancer.
* Sets the number of iterations for Container Balancer.
*
* @param count a idle iteration count larger than 0
* @param count a value greater than 0, or -1
*/
public void setIdleIteration(int count) {
public void setIterations(int count) {
if (count < -1 || 0 == count) {
throw new IllegalArgumentException(
"Idle iteration count must be larger than 0 or " +
"-1(for infinitely running).");
"Iteration count must be greater than 0, or " +
"-1(for running balancer infinitely).");
}
this.idleIterations = count;
this.iterations = count;
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -710,7 +710,7 @@ public StartContainerBalancerResponseProto startContainerBalancer(
StartContainerBalancerRequestProto request)
throws IOException {
Optional<Double> threshold = Optional.empty();
Optional<Integer> idleiterations = Optional.empty();
Optional<Integer> iterations = Optional.empty();
Optional<Double> maxDatanodesRatioToInvolvePerIteration = Optional.empty();
Optional<Long> maxSizeToMovePerIterationInGB = Optional.empty();
Optional<Long> maxSizeEnteringTargetInGB = Optional.empty();
Expand All @@ -719,8 +719,8 @@ public StartContainerBalancerResponseProto startContainerBalancer(
if(request.hasThreshold()) {
threshold = Optional.of(request.getThreshold());
}
if(request.hasIdleiterations()) {
idleiterations = Optional.of(request.getIdleiterations());
if(request.hasIterations()) {
iterations = Optional.of(request.getIterations());
}
if(request.hasMaxDatanodesRatioToInvolvePerIteration()) {
maxDatanodesRatioToInvolvePerIteration =
Expand All @@ -743,7 +743,7 @@ public StartContainerBalancerResponseProto startContainerBalancer(

return StartContainerBalancerResponseProto.newBuilder().
setStart(impl.startContainerBalancer(threshold,
idleiterations, maxDatanodesRatioToInvolvePerIteration,
iterations, maxDatanodesRatioToInvolvePerIteration,
maxSizeToMovePerIterationInGB,
maxSizeEnteringTargetInGB, maxSizeLeavingSourceInGB)).build();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -744,7 +744,7 @@ public StatusAndMessages queryUpgradeFinalizationProgress(

@Override
public boolean startContainerBalancer(
Optional<Double> threshold, Optional<Integer> idleiterations,
Optional<Double> threshold, Optional<Integer> iterations,
Optional<Double> maxDatanodesRatioToInvolvePerIteration,
Optional<Long> maxSizeToMovePerIterationInGB,
Optional<Long> maxSizeEnteringTarget,
Expand Down Expand Up @@ -774,12 +774,12 @@ public boolean startContainerBalancer(
"lesser than or equal to one.");
cbc.setMaxDatanodesRatioToInvolvePerIteration(mdti);
}
if (idleiterations.isPresent()) {
int idi = idleiterations.get();
Preconditions.checkState(idi > 0 || idi == -1,
"idleiterations must be positive or" +
if (iterations.isPresent()) {
int i = iterations.get();
Preconditions.checkState(i > 0 || i == -1,
"iterations must be positive or" +
" -1(infinitly run container balancer).");
cbc.setIdleIteration(idi);
cbc.setIterations(i);
}

if (maxSizeEnteringTarget.isPresent()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ public void setup() throws SCMException, NodeNotFoundException {

balancerConfiguration = new ContainerBalancerConfiguration(conf);
balancerConfiguration.setThreshold(0.1);
balancerConfiguration.setIdleIteration(1);
balancerConfiguration.setIterations(1);
balancerConfiguration.setMaxDatanodesRatioToInvolvePerIteration(1.0d);
balancerConfiguration.setMaxSizeToMovePerIteration(50 * OzoneConsts.GB);
balancerConfiguration.setMaxSizeEnteringTarget(5 * OzoneConsts.GB);
Expand Down Expand Up @@ -231,7 +231,7 @@ public void containerBalancerShouldObeyMaxDatanodesToInvolveLimit() {
balancerConfiguration.setMaxDatanodesRatioToInvolvePerIteration(0.3d);
balancerConfiguration.setMaxSizeToMovePerIteration(100 * OzoneConsts.GB);
balancerConfiguration.setThreshold(0.01);
balancerConfiguration.setIdleIteration(1);
balancerConfiguration.setIterations(1);
containerBalancer.start(balancerConfiguration);

// waiting for balance completed.
Expand Down Expand Up @@ -297,7 +297,7 @@ public void containerBalancerShouldSelectOnlyClosedContainers() {
public void containerBalancerShouldObeyMaxSizeToMoveLimit() {
balancerConfiguration.setThreshold(0.01);
balancerConfiguration.setMaxSizeToMovePerIteration(10 * OzoneConsts.GB);
balancerConfiguration.setIdleIteration(1);
balancerConfiguration.setIterations(1);
containerBalancer.start(balancerConfiguration);

// waiting for balance completed.
Expand Down Expand Up @@ -505,7 +505,7 @@ public void balancerShouldObeyMaxSizeEnteringTargetLimit() {
@Test
public void testMetrics() {
balancerConfiguration.setThreshold(0.1);
balancerConfiguration.setIdleIteration(1);
balancerConfiguration.setIterations(1);
balancerConfiguration.setMaxSizeEnteringTarget(10 * OzoneConsts.GB);
balancerConfiguration.setMaxSizeToMovePerIteration(100 * OzoneConsts.GB);
balancerConfiguration.setMaxDatanodesRatioToInvolvePerIteration(1.0);
Expand Down Expand Up @@ -540,7 +540,7 @@ public void testMetrics() {
@Test
public void balancerShouldFollowExcludeAndIncludeDatanodesConfigurations() {
balancerConfiguration.setThreshold(0.1);
balancerConfiguration.setIdleIteration(1);
balancerConfiguration.setIterations(1);
balancerConfiguration.setMaxSizeEnteringTarget(10 * OzoneConsts.GB);
balancerConfiguration.setMaxSizeToMovePerIteration(100 * OzoneConsts.GB);
balancerConfiguration.setMaxDatanodesRatioToInvolvePerIteration(1.0);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@
* To start:
* ozone admin containerbalancer start
* [ -t/--threshold {@literal <threshold>}]
* [ -i/--idleiterations {@literal <idleiterations>}]
* [ -i/--iterations {@literal <iterations>}]
* [ -d/--maxDatanodesRatioToInvolvePerIteration
* {@literal <maxDatanodesRatioToInvolvePerIteration>}]
* [ -s/--maxSizeToMovePerIterationInGB
Expand All @@ -52,7 +52,7 @@
* ozone admin containerbalancer start -t 0.05
* start balancer with a threshold of 5%
* ozone admin containerbalancer start -i 20
* start balancer with maximum 20 consecutive idle iterations
* start balancer with maximum 20 consecutive iterations
* ozone admin containerbalancer start -i 0
* run balancer infinitely with default values in the configuration
* ozone admin containerbalancer start -d 0.4
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,10 @@ public class ContainerBalancerStartSubcommand extends ScmSubcommand {
description = "Threshold target whether the cluster is balanced")
private Optional<Double> threshold;

@Option(names = {"-i", "--idleiterations"},
description = "Maximum consecutive idle iterations")
private Optional<Integer> idleiterations;
@Option(names = {"-i", "--iterations"},
description = "Maximum consecutive iterations that" +
" balancer will run for")
private Optional<Integer> iterations;

@Option(names = {"-d", "--maxDatanodesRatioToInvolvePerIteration"},
description = "The ratio of maximum number of datanodes that should be " +
Expand All @@ -66,7 +67,7 @@ public class ContainerBalancerStartSubcommand extends ScmSubcommand {

@Override
public void execute(ScmClient scmClient) throws IOException {
boolean result = scmClient.startContainerBalancer(threshold, idleiterations,
boolean result = scmClient.startContainerBalancer(threshold, iterations,
maxDatanodesRatioToInvolvePerIteration, maxSizeToMovePerIterationInGB,
maxSizeEnteringTargetInGB, maxSizeLeavingSourceInGB);
if (result) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -552,14 +552,14 @@ public boolean getReplicationManagerStatus() throws IOException {

@Override
public boolean startContainerBalancer(
Optional<Double> threshold, Optional<Integer> idleiterations,
Optional<Double> threshold, Optional<Integer> iterations,
Optional<Double> maxDatanodesRatioToInvolvePerIteration,
Optional<Long> maxSizeToMovePerIterationInGB,
Optional<Long> maxSizeEnteringTargetInGB,
Optional<Long> maxSizeLeavingSourceInGB)
throws IOException {
return storageContainerLocationClient.startContainerBalancer(threshold,
idleiterations, maxDatanodesRatioToInvolvePerIteration,
iterations, maxDatanodesRatioToInvolvePerIteration,
maxSizeToMovePerIterationInGB, maxSizeEnteringTargetInGB,
maxSizeLeavingSourceInGB);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -81,13 +81,13 @@ public void testContainerBalancerCLIOperations() throws Exception {
boolean running = containerBalancerClient.getContainerBalancerStatus();
assertFalse(running);
Optional<Double> threshold = Optional.of(0.1);
Optional<Integer> idleiterations = Optional.of(10000);
Optional<Integer> iterations = Optional.of(10000);
Optional<Double> maxDatanodesRatioToInvolvePerIteration = Optional.of(1d);
Optional<Long> maxSizeToMovePerIterationInGB = Optional.of(1L);
Optional<Long> maxSizeEnteringTargetInGB = Optional.of(1L);
Optional<Long> maxSizeLeavingSourceInGB = Optional.of(1L);

containerBalancerClient.startContainerBalancer(threshold, idleiterations,
containerBalancerClient.startContainerBalancer(threshold, iterations,
maxDatanodesRatioToInvolvePerIteration, maxSizeToMovePerIterationInGB,
maxSizeEnteringTargetInGB, maxSizeLeavingSourceInGB);
running = containerBalancerClient.getContainerBalancerStatus();
Expand All @@ -104,7 +104,7 @@ public void testContainerBalancerCLIOperations() throws Exception {
assertFalse(running);

// test normally start , and stop it before balance is completed
containerBalancerClient.startContainerBalancer(threshold, idleiterations,
containerBalancerClient.startContainerBalancer(threshold, iterations,
maxDatanodesRatioToInvolvePerIteration, maxSizeToMovePerIterationInGB,
maxSizeEnteringTargetInGB, maxSizeLeavingSourceInGB);
running = containerBalancerClient.getContainerBalancerStatus();
Expand Down