Tejaskriya commented on code in PR #6241:
URL: https://github.com/apache/ozone/pull/6241#discussion_r1537976436
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java:
##########
@@ -1047,67 +1047,131 @@ public StartContainerBalancerResponseProto
startContainerBalancer(
Optional<Integer> maxDatanodesPercentageToInvolvePerIteration,
Optional<Long> maxSizeToMovePerIterationInGB,
Optional<Long> maxSizeEnteringTarget,
- Optional<Long> maxSizeLeavingSource) throws IOException {
+ Optional<Long> maxSizeLeavingSource,
+ Optional<Integer> balancingInterval,
+ Optional<Integer> moveTimeout,
+ Optional<Integer> moveReplicationTimeout,
+ Optional<Boolean> networkTopologyEnable,
+ Optional<String> includeNodes,
+ Optional<String> excludeNodes) throws IOException {
getScm().checkAdminAccess(getRemoteUser(), false);
ContainerBalancerConfiguration cbc =
scm.getConfiguration().getObject(ContainerBalancerConfiguration.class);
Map<String, String> auditMap = Maps.newHashMap();
- if (threshold.isPresent()) {
- double tsd = threshold.get();
- auditMap.put("threshold", String.valueOf(tsd));
- Preconditions.checkState(tsd >= 0.0D && tsd < 100.0D,
- "threshold should be specified in range [0.0, 100.0).");
- cbc.setThreshold(tsd);
- }
- if (maxSizeToMovePerIterationInGB.isPresent()) {
- long mstm = maxSizeToMovePerIterationInGB.get();
- auditMap.put("maxSizeToMovePerIterationInGB", String.valueOf(mstm));
- Preconditions.checkState(mstm > 0,
- "maxSizeToMovePerIterationInGB must be positive.");
- cbc.setMaxSizeToMovePerIteration(mstm * OzoneConsts.GB);
- }
- if (maxDatanodesPercentageToInvolvePerIteration.isPresent()) {
- int mdti = maxDatanodesPercentageToInvolvePerIteration.get();
- auditMap.put("maxDatanodesPercentageToInvolvePerIteration",
- String.valueOf(mdti));
- Preconditions.checkState(mdti >= 0,
- "maxDatanodesPercentageToInvolvePerIteration must be " +
- "greater than equal to zero.");
- Preconditions.checkState(mdti <= 100,
- "maxDatanodesPercentageToInvolvePerIteration must be " +
- "lesser than or equal to 100.");
- cbc.setMaxDatanodesPercentageToInvolvePerIteration(mdti);
- }
- if (iterations.isPresent()) {
- int i = iterations.get();
- auditMap.put("iterations", String.valueOf(i));
- Preconditions.checkState(i > 0 || i == -1,
- "number of iterations must be positive or" +
- " -1 (for running container balancer infinitely).");
- cbc.setIterations(i);
- }
+ ResultCodes resultCodes = ResultCodes.FAILED_TO_START_CONTAINER_BALANCER;
+ try {
+ if (threshold.isPresent()) {
+ double tsd = threshold.get();
+ auditMap.put("threshold", String.valueOf(tsd));
+ if (tsd < 0.0D || tsd >= 100.0D) {
+ throw new SCMException("Threshold should be specified in the range
[0.0, 100.0).", resultCodes);
+ }
+ cbc.setThreshold(tsd);
+ }
- if (maxSizeEnteringTarget.isPresent()) {
- long mset = maxSizeEnteringTarget.get();
- auditMap.put("maxSizeEnteringTarget", String.valueOf(mset));
- Preconditions.checkState(mset > 0,
- "maxSizeEnteringTarget must be " +
- "greater than zero.");
- cbc.setMaxSizeEnteringTarget(mset * OzoneConsts.GB);
- }
+ if (maxSizeToMovePerIterationInGB.isPresent()) {
+ long mstm = maxSizeToMovePerIterationInGB.get();
+ auditMap.put("maxSizeToMovePerIterationInGB", String.valueOf(mstm));
+ if (mstm < 0) {
+ throw new SCMException("Max Size To Move Per Iteration In GB must be
positive.", resultCodes);
+ }
+ cbc.setMaxSizeToMovePerIteration(mstm * OzoneConsts.GB);
+ }
- if (maxSizeLeavingSource.isPresent()) {
- long msls = maxSizeLeavingSource.get();
- auditMap.put("maxSizeLeavingSource", String.valueOf(msls));
- Preconditions.checkState(msls > 0,
- "maxSizeLeavingSource must be " +
- "greater than zero.");
- cbc.setMaxSizeLeavingSource(msls * OzoneConsts.GB);
- }
+ if (maxDatanodesPercentageToInvolvePerIteration.isPresent()) {
+ int mdti = maxDatanodesPercentageToInvolvePerIteration.get();
+ auditMap.put("maxDatanodesPercentageToInvolvePerIteration",
+ String.valueOf(mdti));
+ if (mdti < 0 || mdti > 100) {
+ throw new SCMException("Max Datanodes Percentage To Involve Per
Iteration" +
+ "should be specified in the range [0, 100]", resultCodes);
+ }
+ cbc.setMaxDatanodesPercentageToInvolvePerIteration(mdti);
+ }
- ContainerBalancer containerBalancer = scm.getContainerBalancer();
- try {
+ if (iterations.isPresent()) {
+ int i = iterations.get();
+ auditMap.put("iterations", String.valueOf(i));
+ if (i < -1 || i == 0) {
+ throw new SCMException("Number of Iterations must be positive or" +
+ " -1 (for running container balancer infinitely).", resultCodes);
+ }
+ cbc.setIterations(i);
+ }
+
+ if (maxSizeEnteringTarget.isPresent()) {
+ long mset = maxSizeEnteringTarget.get();
+ auditMap.put("maxSizeEnteringTarget", String.valueOf(mset));
+ if (mset < 0) {
+ throw new SCMException("Max Size Entering Target must be " +
+ "greater than zero.", resultCodes);
+ }
+ cbc.setMaxSizeEnteringTarget(mset * OzoneConsts.GB);
+ }
+
+ if (maxSizeLeavingSource.isPresent()) {
+ long msls = maxSizeLeavingSource.get();
+ auditMap.put("maxSizeLeavingSource", String.valueOf(msls));
+ if (msls < 0) {
Review Comment:
Similar lines, previous check was `msls > 0`
```suggestion
if (msls <= 0) {
```
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java:
##########
@@ -1047,67 +1047,131 @@ public StartContainerBalancerResponseProto
startContainerBalancer(
Optional<Integer> maxDatanodesPercentageToInvolvePerIteration,
Optional<Long> maxSizeToMovePerIterationInGB,
Optional<Long> maxSizeEnteringTarget,
- Optional<Long> maxSizeLeavingSource) throws IOException {
+ Optional<Long> maxSizeLeavingSource,
+ Optional<Integer> balancingInterval,
+ Optional<Integer> moveTimeout,
+ Optional<Integer> moveReplicationTimeout,
+ Optional<Boolean> networkTopologyEnable,
+ Optional<String> includeNodes,
+ Optional<String> excludeNodes) throws IOException {
getScm().checkAdminAccess(getRemoteUser(), false);
ContainerBalancerConfiguration cbc =
scm.getConfiguration().getObject(ContainerBalancerConfiguration.class);
Map<String, String> auditMap = Maps.newHashMap();
- if (threshold.isPresent()) {
- double tsd = threshold.get();
- auditMap.put("threshold", String.valueOf(tsd));
- Preconditions.checkState(tsd >= 0.0D && tsd < 100.0D,
- "threshold should be specified in range [0.0, 100.0).");
- cbc.setThreshold(tsd);
- }
- if (maxSizeToMovePerIterationInGB.isPresent()) {
- long mstm = maxSizeToMovePerIterationInGB.get();
- auditMap.put("maxSizeToMovePerIterationInGB", String.valueOf(mstm));
- Preconditions.checkState(mstm > 0,
- "maxSizeToMovePerIterationInGB must be positive.");
- cbc.setMaxSizeToMovePerIteration(mstm * OzoneConsts.GB);
- }
- if (maxDatanodesPercentageToInvolvePerIteration.isPresent()) {
- int mdti = maxDatanodesPercentageToInvolvePerIteration.get();
- auditMap.put("maxDatanodesPercentageToInvolvePerIteration",
- String.valueOf(mdti));
- Preconditions.checkState(mdti >= 0,
- "maxDatanodesPercentageToInvolvePerIteration must be " +
- "greater than equal to zero.");
- Preconditions.checkState(mdti <= 100,
- "maxDatanodesPercentageToInvolvePerIteration must be " +
- "lesser than or equal to 100.");
- cbc.setMaxDatanodesPercentageToInvolvePerIteration(mdti);
- }
- if (iterations.isPresent()) {
- int i = iterations.get();
- auditMap.put("iterations", String.valueOf(i));
- Preconditions.checkState(i > 0 || i == -1,
- "number of iterations must be positive or" +
- " -1 (for running container balancer infinitely).");
- cbc.setIterations(i);
- }
+ ResultCodes resultCodes = ResultCodes.FAILED_TO_START_CONTAINER_BALANCER;
+ try {
+ if (threshold.isPresent()) {
+ double tsd = threshold.get();
+ auditMap.put("threshold", String.valueOf(tsd));
+ if (tsd < 0.0D || tsd >= 100.0D) {
+ throw new SCMException("Threshold should be specified in the range
[0.0, 100.0).", resultCodes);
+ }
+ cbc.setThreshold(tsd);
+ }
- if (maxSizeEnteringTarget.isPresent()) {
- long mset = maxSizeEnteringTarget.get();
- auditMap.put("maxSizeEnteringTarget", String.valueOf(mset));
- Preconditions.checkState(mset > 0,
- "maxSizeEnteringTarget must be " +
- "greater than zero.");
- cbc.setMaxSizeEnteringTarget(mset * OzoneConsts.GB);
- }
+ if (maxSizeToMovePerIterationInGB.isPresent()) {
+ long mstm = maxSizeToMovePerIterationInGB.get();
+ auditMap.put("maxSizeToMovePerIterationInGB", String.valueOf(mstm));
+ if (mstm < 0) {
Review Comment:
As the check earlier was `mstm > 0`
```suggestion
if (mstm <= 0) {
```
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java:
##########
@@ -1047,67 +1047,131 @@ public StartContainerBalancerResponseProto
startContainerBalancer(
Optional<Integer> maxDatanodesPercentageToInvolvePerIteration,
Optional<Long> maxSizeToMovePerIterationInGB,
Optional<Long> maxSizeEnteringTarget,
- Optional<Long> maxSizeLeavingSource) throws IOException {
+ Optional<Long> maxSizeLeavingSource,
+ Optional<Integer> balancingInterval,
+ Optional<Integer> moveTimeout,
+ Optional<Integer> moveReplicationTimeout,
+ Optional<Boolean> networkTopologyEnable,
+ Optional<String> includeNodes,
+ Optional<String> excludeNodes) throws IOException {
getScm().checkAdminAccess(getRemoteUser(), false);
ContainerBalancerConfiguration cbc =
scm.getConfiguration().getObject(ContainerBalancerConfiguration.class);
Map<String, String> auditMap = Maps.newHashMap();
- if (threshold.isPresent()) {
- double tsd = threshold.get();
- auditMap.put("threshold", String.valueOf(tsd));
- Preconditions.checkState(tsd >= 0.0D && tsd < 100.0D,
- "threshold should be specified in range [0.0, 100.0).");
- cbc.setThreshold(tsd);
- }
- if (maxSizeToMovePerIterationInGB.isPresent()) {
- long mstm = maxSizeToMovePerIterationInGB.get();
- auditMap.put("maxSizeToMovePerIterationInGB", String.valueOf(mstm));
- Preconditions.checkState(mstm > 0,
- "maxSizeToMovePerIterationInGB must be positive.");
- cbc.setMaxSizeToMovePerIteration(mstm * OzoneConsts.GB);
- }
- if (maxDatanodesPercentageToInvolvePerIteration.isPresent()) {
- int mdti = maxDatanodesPercentageToInvolvePerIteration.get();
- auditMap.put("maxDatanodesPercentageToInvolvePerIteration",
- String.valueOf(mdti));
- Preconditions.checkState(mdti >= 0,
- "maxDatanodesPercentageToInvolvePerIteration must be " +
- "greater than equal to zero.");
- Preconditions.checkState(mdti <= 100,
- "maxDatanodesPercentageToInvolvePerIteration must be " +
- "lesser than or equal to 100.");
- cbc.setMaxDatanodesPercentageToInvolvePerIteration(mdti);
- }
- if (iterations.isPresent()) {
- int i = iterations.get();
- auditMap.put("iterations", String.valueOf(i));
- Preconditions.checkState(i > 0 || i == -1,
- "number of iterations must be positive or" +
- " -1 (for running container balancer infinitely).");
- cbc.setIterations(i);
- }
+ ResultCodes resultCodes = ResultCodes.FAILED_TO_START_CONTAINER_BALANCER;
+ try {
+ if (threshold.isPresent()) {
+ double tsd = threshold.get();
+ auditMap.put("threshold", String.valueOf(tsd));
+ if (tsd < 0.0D || tsd >= 100.0D) {
+ throw new SCMException("Threshold should be specified in the range
[0.0, 100.0).", resultCodes);
+ }
+ cbc.setThreshold(tsd);
+ }
- if (maxSizeEnteringTarget.isPresent()) {
- long mset = maxSizeEnteringTarget.get();
- auditMap.put("maxSizeEnteringTarget", String.valueOf(mset));
- Preconditions.checkState(mset > 0,
- "maxSizeEnteringTarget must be " +
- "greater than zero.");
- cbc.setMaxSizeEnteringTarget(mset * OzoneConsts.GB);
- }
+ if (maxSizeToMovePerIterationInGB.isPresent()) {
+ long mstm = maxSizeToMovePerIterationInGB.get();
+ auditMap.put("maxSizeToMovePerIterationInGB", String.valueOf(mstm));
+ if (mstm < 0) {
+ throw new SCMException("Max Size To Move Per Iteration In GB must be
positive.", resultCodes);
+ }
+ cbc.setMaxSizeToMovePerIteration(mstm * OzoneConsts.GB);
+ }
- if (maxSizeLeavingSource.isPresent()) {
- long msls = maxSizeLeavingSource.get();
- auditMap.put("maxSizeLeavingSource", String.valueOf(msls));
- Preconditions.checkState(msls > 0,
- "maxSizeLeavingSource must be " +
- "greater than zero.");
- cbc.setMaxSizeLeavingSource(msls * OzoneConsts.GB);
- }
+ if (maxDatanodesPercentageToInvolvePerIteration.isPresent()) {
+ int mdti = maxDatanodesPercentageToInvolvePerIteration.get();
+ auditMap.put("maxDatanodesPercentageToInvolvePerIteration",
+ String.valueOf(mdti));
+ if (mdti < 0 || mdti > 100) {
+ throw new SCMException("Max Datanodes Percentage To Involve Per
Iteration" +
+ "should be specified in the range [0, 100]", resultCodes);
+ }
+ cbc.setMaxDatanodesPercentageToInvolvePerIteration(mdti);
+ }
- ContainerBalancer containerBalancer = scm.getContainerBalancer();
- try {
+ if (iterations.isPresent()) {
+ int i = iterations.get();
+ auditMap.put("iterations", String.valueOf(i));
+ if (i < -1 || i == 0) {
+ throw new SCMException("Number of Iterations must be positive or" +
+ " -1 (for running container balancer infinitely).", resultCodes);
+ }
+ cbc.setIterations(i);
+ }
+
+ if (maxSizeEnteringTarget.isPresent()) {
+ long mset = maxSizeEnteringTarget.get();
+ auditMap.put("maxSizeEnteringTarget", String.valueOf(mset));
+ if (mset < 0) {
+ throw new SCMException("Max Size Entering Target must be " +
+ "greater than zero.", resultCodes);
+ }
+ cbc.setMaxSizeEnteringTarget(mset * OzoneConsts.GB);
+ }
+
+ if (maxSizeLeavingSource.isPresent()) {
+ long msls = maxSizeLeavingSource.get();
+ auditMap.put("maxSizeLeavingSource", String.valueOf(msls));
+ if (msls < 0) {
+ throw new SCMException("Max Size Leaving Source must be " +
+ "greater than zero.", resultCodes);
+ }
+ cbc.setMaxSizeLeavingSource(msls * OzoneConsts.GB);
+ }
+
+ if (balancingInterval.isPresent()) {
+ int bi = balancingInterval.get();
+ auditMap.put("balancingInterval", String.valueOf(bi));
+ if (bi < 0) {
Review Comment:
Can balancingInterval be set to zero? If not, then the check should be `bi
<= 0`. Same for moveTimeout and moveReplicationTimeout in the following lines
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java:
##########
@@ -1047,67 +1047,131 @@ public StartContainerBalancerResponseProto
startContainerBalancer(
Optional<Integer> maxDatanodesPercentageToInvolvePerIteration,
Optional<Long> maxSizeToMovePerIterationInGB,
Optional<Long> maxSizeEnteringTarget,
- Optional<Long> maxSizeLeavingSource) throws IOException {
+ Optional<Long> maxSizeLeavingSource,
+ Optional<Integer> balancingInterval,
+ Optional<Integer> moveTimeout,
+ Optional<Integer> moveReplicationTimeout,
+ Optional<Boolean> networkTopologyEnable,
+ Optional<String> includeNodes,
+ Optional<String> excludeNodes) throws IOException {
getScm().checkAdminAccess(getRemoteUser(), false);
ContainerBalancerConfiguration cbc =
scm.getConfiguration().getObject(ContainerBalancerConfiguration.class);
Map<String, String> auditMap = Maps.newHashMap();
- if (threshold.isPresent()) {
- double tsd = threshold.get();
- auditMap.put("threshold", String.valueOf(tsd));
- Preconditions.checkState(tsd >= 0.0D && tsd < 100.0D,
- "threshold should be specified in range [0.0, 100.0).");
- cbc.setThreshold(tsd);
- }
- if (maxSizeToMovePerIterationInGB.isPresent()) {
- long mstm = maxSizeToMovePerIterationInGB.get();
- auditMap.put("maxSizeToMovePerIterationInGB", String.valueOf(mstm));
- Preconditions.checkState(mstm > 0,
- "maxSizeToMovePerIterationInGB must be positive.");
- cbc.setMaxSizeToMovePerIteration(mstm * OzoneConsts.GB);
- }
- if (maxDatanodesPercentageToInvolvePerIteration.isPresent()) {
- int mdti = maxDatanodesPercentageToInvolvePerIteration.get();
- auditMap.put("maxDatanodesPercentageToInvolvePerIteration",
- String.valueOf(mdti));
- Preconditions.checkState(mdti >= 0,
- "maxDatanodesPercentageToInvolvePerIteration must be " +
- "greater than equal to zero.");
- Preconditions.checkState(mdti <= 100,
- "maxDatanodesPercentageToInvolvePerIteration must be " +
- "lesser than or equal to 100.");
- cbc.setMaxDatanodesPercentageToInvolvePerIteration(mdti);
- }
- if (iterations.isPresent()) {
- int i = iterations.get();
- auditMap.put("iterations", String.valueOf(i));
- Preconditions.checkState(i > 0 || i == -1,
- "number of iterations must be positive or" +
- " -1 (for running container balancer infinitely).");
- cbc.setIterations(i);
- }
+ ResultCodes resultCodes = ResultCodes.FAILED_TO_START_CONTAINER_BALANCER;
+ try {
+ if (threshold.isPresent()) {
+ double tsd = threshold.get();
+ auditMap.put("threshold", String.valueOf(tsd));
+ if (tsd < 0.0D || tsd >= 100.0D) {
+ throw new SCMException("Threshold should be specified in the range
[0.0, 100.0).", resultCodes);
+ }
+ cbc.setThreshold(tsd);
+ }
- if (maxSizeEnteringTarget.isPresent()) {
- long mset = maxSizeEnteringTarget.get();
- auditMap.put("maxSizeEnteringTarget", String.valueOf(mset));
- Preconditions.checkState(mset > 0,
- "maxSizeEnteringTarget must be " +
- "greater than zero.");
- cbc.setMaxSizeEnteringTarget(mset * OzoneConsts.GB);
- }
+ if (maxSizeToMovePerIterationInGB.isPresent()) {
+ long mstm = maxSizeToMovePerIterationInGB.get();
+ auditMap.put("maxSizeToMovePerIterationInGB", String.valueOf(mstm));
+ if (mstm < 0) {
+ throw new SCMException("Max Size To Move Per Iteration In GB must be
positive.", resultCodes);
+ }
+ cbc.setMaxSizeToMovePerIteration(mstm * OzoneConsts.GB);
+ }
- if (maxSizeLeavingSource.isPresent()) {
- long msls = maxSizeLeavingSource.get();
- auditMap.put("maxSizeLeavingSource", String.valueOf(msls));
- Preconditions.checkState(msls > 0,
- "maxSizeLeavingSource must be " +
- "greater than zero.");
- cbc.setMaxSizeLeavingSource(msls * OzoneConsts.GB);
- }
+ if (maxDatanodesPercentageToInvolvePerIteration.isPresent()) {
+ int mdti = maxDatanodesPercentageToInvolvePerIteration.get();
+ auditMap.put("maxDatanodesPercentageToInvolvePerIteration",
+ String.valueOf(mdti));
+ if (mdti < 0 || mdti > 100) {
+ throw new SCMException("Max Datanodes Percentage To Involve Per
Iteration" +
+ "should be specified in the range [0, 100]", resultCodes);
+ }
+ cbc.setMaxDatanodesPercentageToInvolvePerIteration(mdti);
+ }
- ContainerBalancer containerBalancer = scm.getContainerBalancer();
- try {
+ if (iterations.isPresent()) {
+ int i = iterations.get();
+ auditMap.put("iterations", String.valueOf(i));
+ if (i < -1 || i == 0) {
+ throw new SCMException("Number of Iterations must be positive or" +
+ " -1 (for running container balancer infinitely).", resultCodes);
+ }
+ cbc.setIterations(i);
+ }
+
+ if (maxSizeEnteringTarget.isPresent()) {
+ long mset = maxSizeEnteringTarget.get();
+ auditMap.put("maxSizeEnteringTarget", String.valueOf(mset));
+ if (mset < 0) {
Review Comment:
As the previous check was `mset > 0`
```suggestion
if (mset <= 0) {
```
--
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]
