siddhantsangwan commented on code in PR #6241:
URL: https://github.com/apache/ozone/pull/6241#discussion_r1499005366
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/scm/protocolPB/StorageContainerLocationProtocolClientSideTranslatorPB.java:
##########
@@ -952,6 +958,48 @@ public StartContainerBalancerResponseProto
startContainerBalancer(
builder.setMaxSizeLeavingSourceInGB(msls);
}
+ if (balancingInterval.isPresent()) {
+ long bi = balancingInterval.get();
+ Preconditions.checkState(bi > 0,
+ "balancingInterval must be greater than zero.");
+ builder.setBalancingInterval(bi);
+ }
+
+ if (moveTimeout.isPresent()) {
+ long mt = moveTimeout.get();
+ Preconditions.checkState(mt > 0,
+ "moveTimeout must be greater than zero.");
+ builder.setMoveTimeout(mt);
+ }
+
+ if (moveReplicationTimeout.isPresent()) {
+ long mrt = moveReplicationTimeout.get();
+ Preconditions.checkState(mrt > 0,
+ "moveTimeout must be greater than zero.");
+ builder.setMoveReplicationTimeout(mrt);
+ }
+
+ if (networkTopologyEnable.isPresent()) {
+ Boolean nt = networkTopologyEnable.get();
+ Preconditions.checkState(nt != null,
+ "networkTopologyEnable must be either true or false");
Review Comment:
This Preconditions check isn't required since we're already checking whether
`networkTopologyEnable` contains a non null value in line 982 above.
##########
hadoop-hdds/tools/src/main/java/org/apache/hadoop/hdds/scm/cli/ContainerBalancerStartSubcommand.java:
##########
@@ -74,13 +74,54 @@ public class ContainerBalancerStartSubcommand extends
ScmSubcommand {
"(for example, '26' for 26GB).")
private Optional<Long> maxSizeLeavingSourceInGB;
+ @Option(names = {"-b", "--balancing-iteration-interval"},
Review Comment:
How about we call this `--balancing-iteration-interval-minutes` to indicate
we're expecting time in minutes? We can also add that to the description to
make it super clear. Same for the other time related options.
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/scm/protocolPB/StorageContainerLocationProtocolClientSideTranslatorPB.java:
##########
@@ -952,6 +958,48 @@ public StartContainerBalancerResponseProto
startContainerBalancer(
builder.setMaxSizeLeavingSourceInGB(msls);
}
+ if (balancingInterval.isPresent()) {
+ long bi = balancingInterval.get();
+ Preconditions.checkState(bi > 0,
+ "balancingInterval must be greater than zero.");
+ builder.setBalancingInterval(bi);
+ }
+
+ if (moveTimeout.isPresent()) {
+ long mt = moveTimeout.get();
+ Preconditions.checkState(mt > 0,
+ "moveTimeout must be greater than zero.");
+ builder.setMoveTimeout(mt);
+ }
+
+ if (moveReplicationTimeout.isPresent()) {
+ long mrt = moveReplicationTimeout.get();
+ Preconditions.checkState(mrt > 0,
+ "moveTimeout must be greater than zero.");
+ builder.setMoveReplicationTimeout(mrt);
+ }
+
+ if (networkTopologyEnable.isPresent()) {
+ Boolean nt = networkTopologyEnable.get();
+ Preconditions.checkState(nt != null,
+ "networkTopologyEnable must be either true or false");
+ builder.setNetworkTopologyEnable(nt);
+ }
+
+ if (includeNodes.isPresent()) {
+ String in = includeNodes.get();
+ Preconditions.checkState(in != null,
+ "includeNodes must contain comma separated hostnames or ip
addresses");
+ builder.setIncludeNodes(in);
+ }
+
+ if (excludeNodes.isPresent()) {
+ String ex = excludeNodes.get();
+ Preconditions.checkState(ex != null,
+ "excludeNodes must contain comma separated hostnames or ip
addresses");
Review Comment:
Not required, same as above.
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/scm/protocolPB/StorageContainerLocationProtocolClientSideTranslatorPB.java:
##########
@@ -952,6 +958,48 @@ public StartContainerBalancerResponseProto
startContainerBalancer(
builder.setMaxSizeLeavingSourceInGB(msls);
}
+ if (balancingInterval.isPresent()) {
+ long bi = balancingInterval.get();
+ Preconditions.checkState(bi > 0,
+ "balancingInterval must be greater than zero.");
+ builder.setBalancingInterval(bi);
+ }
+
+ if (moveTimeout.isPresent()) {
+ long mt = moveTimeout.get();
+ Preconditions.checkState(mt > 0,
+ "moveTimeout must be greater than zero.");
+ builder.setMoveTimeout(mt);
+ }
+
+ if (moveReplicationTimeout.isPresent()) {
+ long mrt = moveReplicationTimeout.get();
+ Preconditions.checkState(mrt > 0,
+ "moveTimeout must be greater than zero.");
+ builder.setMoveReplicationTimeout(mrt);
+ }
+
+ if (networkTopologyEnable.isPresent()) {
+ Boolean nt = networkTopologyEnable.get();
+ Preconditions.checkState(nt != null,
+ "networkTopologyEnable must be either true or false");
+ builder.setNetworkTopologyEnable(nt);
+ }
+
+ if (includeNodes.isPresent()) {
+ String in = includeNodes.get();
+ Preconditions.checkState(in != null,
+ "includeNodes must contain comma separated hostnames or ip
addresses");
Review Comment:
Not required and should be removed, same as above. Moreover it isn't
mandatory to specify this option.
##########
hadoop-hdds/tools/src/main/java/org/apache/hadoop/hdds/scm/cli/ContainerBalancerStartSubcommand.java:
##########
@@ -74,13 +74,54 @@ public class ContainerBalancerStartSubcommand extends
ScmSubcommand {
"(for example, '26' for 26GB).")
private Optional<Long> maxSizeLeavingSourceInGB;
+ @Option(names = {"-b", "--balancing-iteration-interval"},
+ description = "The interval period between each iteration of Container
Balancer." +
+ "(for example, '70' for 70m).")
Review Comment:
```suggestion
"(for example, '70' for 70 minutes).")
```
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerConfiguration.java:
##########
@@ -330,16 +330,16 @@ public void setMoveTimeout(Duration duration) {
this.moveTimeout = duration.toMillis();
}
- public void setMoveTimeout(long millis) {
- this.moveTimeout = millis;
+ public void setMoveTimeout(long minutes) {
Review Comment:
Yeah, this setter is meant to accept millis. We should convert minutes to
millis before passing it here. Or use the other setters that accept a Duration.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java:
##########
@@ -1105,6 +1111,54 @@ public StartContainerBalancerResponseProto
startContainerBalancer(
cbc.setMaxSizeLeavingSource(msls * OzoneConsts.GB);
}
+ if (balancingInterval.isPresent()) {
+ long bi = balancingInterval.get();
+ auditMap.put("balancingInterval", String.valueOf(bi));
+ Preconditions.checkState(bi > 0,
+ "balancingInterval must be greater than zero.");
+ cbc.setBalancingInterval(bi);
+ }
+
+ if (moveTimeout.isPresent()) {
+ long mt = moveTimeout.get();
+ auditMap.put("moveTimeout", String.valueOf(mt));
+ Preconditions.checkState(mt > 0,
+ "moveTimeout must be greater than zero.");
+ cbc.setMoveTimeout(mt);
+ }
+
+ if (moveReplicationTimeout.isPresent()) {
+ long mrt = moveReplicationTimeout.get();
+ auditMap.put("moveReplicationTimeout", String.valueOf(mrt));
+ Preconditions.checkState(mrt > 0,
+ "moveTimeout must be greater than zero.");
+ cbc.setMoveReplicationTimeout(mrt);
+ }
+
+ if (networkTopologyEnable.isPresent()) {
+ Boolean nt = networkTopologyEnable.get();
+ auditMap.put("networkTopologyEnable", String.valueOf(nt));
+ Preconditions.checkState(nt != null,
+ "networkTopologyEnable must be either true or false");
Review Comment:
Not required.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java:
##########
@@ -1105,6 +1111,54 @@ public StartContainerBalancerResponseProto
startContainerBalancer(
cbc.setMaxSizeLeavingSource(msls * OzoneConsts.GB);
}
+ if (balancingInterval.isPresent()) {
+ long bi = balancingInterval.get();
+ auditMap.put("balancingInterval", String.valueOf(bi));
+ Preconditions.checkState(bi > 0,
+ "balancingInterval must be greater than zero.");
+ cbc.setBalancingInterval(bi);
+ }
+
+ if (moveTimeout.isPresent()) {
+ long mt = moveTimeout.get();
+ auditMap.put("moveTimeout", String.valueOf(mt));
+ Preconditions.checkState(mt > 0,
+ "moveTimeout must be greater than zero.");
+ cbc.setMoveTimeout(mt);
+ }
+
+ if (moveReplicationTimeout.isPresent()) {
+ long mrt = moveReplicationTimeout.get();
+ auditMap.put("moveReplicationTimeout", String.valueOf(mrt));
+ Preconditions.checkState(mrt > 0,
+ "moveTimeout must be greater than zero.");
+ cbc.setMoveReplicationTimeout(mrt);
+ }
+
+ if (networkTopologyEnable.isPresent()) {
+ Boolean nt = networkTopologyEnable.get();
+ auditMap.put("networkTopologyEnable", String.valueOf(nt));
+ Preconditions.checkState(nt != null,
+ "networkTopologyEnable must be either true or false");
+ cbc.setNetworkTopologyEnable(nt);
+ }
+
+ if (includeNodes.isPresent()) {
+ String in = includeNodes.get();
+ auditMap.put("includeNodes", (in));
+ Preconditions.checkState(in != null,
+ "includeNodes must contain comma separated hostnames or ip
addresses");
+ cbc.setIncludeNodes(in);
+ }
+
+ if (excludeNodes.isPresent()) {
+ String ex = excludeNodes.get();
+ auditMap.put("excludeNodes", (ex));
+ Preconditions.checkState(ex != null,
+ "includeNodes must contain comma separated hostnames or ip
addresses");
Review Comment:
These `Preconditions` checks aren't required either.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
