You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by GitBox <gi...@apache.org> on 2021/05/24 06:08:25 UTC
[GitHub] [ozone] JacksonYao287 opened a new pull request #2278: HDDS-4926. Support start/stop for container balancer via command line
JacksonYao287 opened a new pull request #2278:
URL: https://github.com/apache/ozone/pull/2278
## What changes were proposed in this pull request?
Support start/stop for container balancer via command line
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-4926
## How was this patch tested?
no additional tests
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] lokeshj1703 commented on a change in pull request #2278: HDDS-4926. Support start/stop for container balancer via command line
Posted by GitBox <gi...@apache.org>.
lokeshj1703 commented on a change in pull request #2278:
URL: https://github.com/apache/ozone/pull/2278#discussion_r645652848
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java
##########
@@ -111,27 +112,42 @@ public boolean start(ContainerBalancerConfiguration balancerConfiguration) {
ozoneConfiguration = new OzoneConfiguration();
this.config = balancerConfiguration;
+ this.idleIteration = config.getIdleIteration();
this.threshold = config.getThreshold();
this.maxDatanodesToBalance = config.getMaxDatanodesToBalance();
this.maxSizeToMove = config.getMaxSizeToMove();
this.unBalancedNodes = new ArrayList<>();
-
LOG.info("Starting Container Balancer...{}", this);
- balance();
+ //we should start a new balancer thread async
+ //and response to cli as soon as possible
+
+
+ //this is a temporary implementation
Review comment:
NIT: Can we specify TODO: in the comment?
##########
File path: hadoop-hdds/tools/src/main/java/org/apache/hadoop/hdds/scm/cli/ContainerBalancerStartSubcommand.java
##########
@@ -0,0 +1,64 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hdds.scm.cli;
+
+import org.apache.hadoop.hdds.cli.HddsVersionProvider;
+import org.apache.hadoop.hdds.scm.client.ScmClient;
+import picocli.CommandLine;
+import picocli.CommandLine.Command;
+
+import java.io.IOException;
+
+/**
+ * Handler to start container balancer.
+ */
+@Command(
+ name = "start",
+ description = "Start ContainerBalancer",
+ mixinStandardHelpOptions = true,
+ versionProvider = HddsVersionProvider.class)
+public class ContainerBalancerStartSubcommand extends ScmSubcommand {
+
+ @CommandLine.Option(names = {"-t", "--threshold"},
+ description = "Threshold target whether the cluster is balanced")
+ private double threshold;
Review comment:
We can specify all these options as Optional<Double> etc. The SCMClient could also use Optional as parameter.
##########
File path: hadoop-hdds/interface-admin/src/main/proto/ScmAdminProtocol.proto
##########
@@ -445,6 +454,32 @@ message GetContainerTokenResponseProto {
required TokenProto token = 1;
}
+message StartContainerBalancerRequestProto {
+ optional string traceID = 1;
+ required double threshold = 2;
Review comment:
I think @ChenSammi is pointing out that since all these fields do not have default values they should be optional.
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java
##########
@@ -323,8 +344,22 @@ public static double calculateUtilization(
* Stops ContainerBalancer.
*/
public void stop() {
Review comment:
We will need to make start and stop synchronized. In a race condition it is possible for balancerRunning to be set as true but thread may not be running.
##########
File path: hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/balancer/TestContainerBalancer.java
##########
@@ -96,11 +97,18 @@ public void setup() {
balancerConfiguration.setThreshold(randomThreshold);
containerBalancer.start(balancerConfiguration);
+
+ // waiting for balance completed.
+ // this is a temporary implementation for now
+ // modify this after balancer is fully completed
Review comment:
NIT: Can we add a TODO: in comments?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] siddhantsangwan commented on a change in pull request #2278: HDDS-4926. Support start/stop for container balancer via command line
Posted by GitBox <gi...@apache.org>.
siddhantsangwan commented on a change in pull request #2278:
URL: https://github.com/apache/ozone/pull/2278#discussion_r656778231
##########
File path: hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/scm/protocolPB/StorageContainerLocationProtocolClientSideTranslatorPB.java
##########
@@ -712,6 +718,73 @@ public boolean getReplicationManagerStatus() throws IOException {
}
+ @Override
+ public boolean startContainerBalancer(Optional<Double> threshold,
+ Optional<Integer> idleiterations,
+ Optional<Integer> maxDatanodesToBalance,
+ Optional<Long> maxSizeToMoveInGB) throws IOException{
+ StartContainerBalancerRequestProto.Builder builder =
+ StartContainerBalancerRequestProto.newBuilder();
+ builder.setTraceID(TracingUtil.exportCurrentSpan());
+
+ //make balancer configuration optional
+ if (threshold.isPresent()) {
+ double tsd = threshold.get();
+ Preconditions.checkState(tsd >= 0.0D && tsd < 1.0D,
+ "threshold should to be specified in range [0.0, 1.0).");
+ builder.setThreshold(tsd);
+ }
+ if (maxSizeToMoveInGB.isPresent()) {
+ long mstm = maxSizeToMoveInGB.get();
+ Preconditions.checkState(mstm > 0,
+ "maxSizeToMoveInGB must be positive.");
+ builder.setMaxSizeToMoveInGB(mstm);
+ }
+ if (maxDatanodesToBalance.isPresent()) {
+ int mdtb = maxDatanodesToBalance.get();
+ Preconditions.checkState(mdtb > 0,
+ "maxDatanodesToBalance must be positive.");
+ builder.setMaxDatanodesToBalance(mdtb);
+ }
+ if (idleiterations.isPresent()) {
+ int idi = idleiterations.get();
+ Preconditions.checkState(idi > 0 || idi == -1,
+ "maxDatanodesToBalance must be positive or" +
Review comment:
Did you mean:
```suggestion
"idleIterations must be positive or" +
```
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerConfiguration.java
##########
@@ -78,6 +83,28 @@ public void setThreshold(double threshold) {
this.threshold = String.valueOf(threshold);
}
+ /**
+ * Gets the idle iteration value for Container Balancer.
+ *
+ * @return a idle iteration count larger than 0
+ */
+ public int getIdleIteration() {
+ return idleIterations;
+ }
+
+ /**
+ * Sets the idle iteration value for Container Balancer.
+ *
+ * @param count a idle iteration count larger than 0
+ */
+ public void setIdleIteration(int count) {
+ if (count < 1) {
Review comment:
Aren't we also allowing `idleIterations` to be `-1`? In that case this condition needs to be changed.
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java
##########
@@ -670,6 +674,65 @@ public boolean getReplicationManagerStatus() {
return scm.getReplicationManager().isRunning();
}
+ @Override
+ public boolean startContainerBalancer(Optional<Double> threshold,
+ Optional<Integer> idleiterations,
+ Optional<Integer> maxDatanodesToBalance,
+ Optional<Long> maxSizeToMoveInGB) throws IOException{
+ getScm().checkAdminAccess(getRemoteUser());
+ ContainerBalancerConfiguration cbc = new ContainerBalancerConfiguration();
+ if (threshold.isPresent()) {
+ double tsd = threshold.get();
+ Preconditions.checkState(tsd >= 0.0D && tsd < 1.0D,
+ "threshold should to be specified in range [0.0, 1.0).");
+ cbc.setThreshold(tsd);
+ }
+ if (maxSizeToMoveInGB.isPresent()) {
+ long mstm = maxSizeToMoveInGB.get();
+ Preconditions.checkState(mstm > 0,
+ "maxSizeToMoveInGB must be positive.");
+ cbc.setMaxSizeToMove(mstm * OzoneConsts.GB);
+ }
+ if (maxDatanodesToBalance.isPresent()) {
+ int mdtb = maxDatanodesToBalance.get();
+ Preconditions.checkState(mdtb > 0,
+ "maxDatanodesToBalance must be positive.");
+ cbc.setMaxDatanodesToBalance(mdtb);
+ }
+ if (idleiterations.isPresent()) {
+ int idi = idleiterations.get();
+ Preconditions.checkState(idi > 0 || idi == -1,
+ "maxDatanodesToBalance must be positive or" +
Review comment:
Did you mean:
```suggestion
"idleIterations must be positive or" +
```
##########
File path: hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/balancer/TestContainerBalancer.java
##########
@@ -134,11 +142,17 @@ public void unBalancedNodesListShouldBeEmptyWhenClusterIsBalanced() {
@Test
public void containerBalancerShouldStopWhenMaxDatanodesToBalanceIsReached() {
balancerConfiguration.setMaxDatanodesToBalance(2);
- balancerConfiguration.setThreshold(0);
+ balancerConfiguration.setThreshold(1);
Review comment:
Why is `threshold` set to `1` here? A threshold of 1 means balancing is not required, which is incorrect for this test since we are checking the `maxDatanodesToBalance` configuration. Moreover threshold belongs to `[0, 1)`.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] JacksonYao287 commented on a change in pull request #2278: HDDS-4926. Support start/stop for container balancer via command line
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on a change in pull request #2278:
URL: https://github.com/apache/ozone/pull/2278#discussion_r645977910
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java
##########
@@ -111,27 +112,42 @@ public boolean start(ContainerBalancerConfiguration balancerConfiguration) {
ozoneConfiguration = new OzoneConfiguration();
this.config = balancerConfiguration;
+ this.idleIteration = config.getIdleIteration();
this.threshold = config.getThreshold();
this.maxDatanodesToBalance = config.getMaxDatanodesToBalance();
this.maxSizeToMove = config.getMaxSizeToMove();
this.unBalancedNodes = new ArrayList<>();
-
LOG.info("Starting Container Balancer...{}", this);
- balance();
+ //we should start a new balancer thread async
+ //and response to cli as soon as possible
+
+
+ //this is a temporary implementation
Review comment:
thanks ,will updated these!
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai commented on pull request #2278: HDDS-4926. Support start/stop for container balancer via command line
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on pull request #2278:
URL: https://github.com/apache/ozone/pull/2278#issuecomment-853676434
> Hey，can you help having a look at this CI problem `TestReportPublisher.testCRLStatusReportPublisher`? it seems nothing to do with my PR, i do not modify any CRL related code.
Yes, it's unrelated. #2302 fixes it. Please wait for that to be merged first.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] JacksonYao287 commented on a change in pull request #2278: HDDS-4926. Support start/stop for container balancer via command line
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on a change in pull request #2278:
URL: https://github.com/apache/ozone/pull/2278#discussion_r651487148
##########
File path: hadoop-hdds/tools/src/main/java/org/apache/hadoop/hdds/scm/cli/ContainerBalancerStartSubcommand.java
##########
@@ -39,7 +39,12 @@
private double threshold;
@Option(names = {"-i", "--idleiterations"},
- description = "Maximum consecutive idle iterations")
+ description = "Maximum consecutive idle iterations",
+ //idleiteration could be as follows:
+ // 0 ： indicate running infinitely
+ // positive number ： indicate the specific iterations
+ // -1 : user does not set this value
+ defaultValue = "-1")
Review comment:
yes, this is indeed a problem, which i tried to assign a default value to solve it, but missed in the lastest commit. i see the doc [here](https://picocli.info/#_optionalt), i think here we should use an optional as you said. thanks, i will update
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] lokeshj1703 commented on a change in pull request #2278: HDDS-4926. Support start/stop for container balancer via command line
Posted by GitBox <gi...@apache.org>.
lokeshj1703 commented on a change in pull request #2278:
URL: https://github.com/apache/ozone/pull/2278#discussion_r650016785
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java
##########
@@ -675,17 +676,21 @@ public boolean startContainerBalancer(double threshold, int idleiterations,
AUDIT.logWriteSuccess(buildAuditMessageForSuccess(
SCMAction.START_CONTAINER_BALANCER, null));
ContainerBalancerConfiguration cbc = new ContainerBalancerConfiguration();
+
if (threshold > 0) {
cbc.setThreshold(threshold);
}
if (maxDatanodesToBalance > 0) {
cbc.setMaxDatanodesToBalance(maxDatanodesToBalance);
}
if (maxSizeToMove > 0) {
- cbc.setMaxSizeToMove(maxSizeToMove);
+ cbc.setMaxSizeToMove(maxSizeToMove * OzoneConsts.GB);
Review comment:
Can we also update the description for option maxSizeToMove? We can mention that for 10GB it should be mentioned as 10. I think it would be good to update the option name to maxSizeToMoveInGB.
##########
File path: hadoop-hdds/tools/src/main/java/org/apache/hadoop/hdds/scm/cli/ContainerBalancerStartSubcommand.java
##########
@@ -0,0 +1,69 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hdds.scm.cli;
+
+import org.apache.hadoop.hdds.cli.HddsVersionProvider;
+import org.apache.hadoop.hdds.scm.client.ScmClient;
+import picocli.CommandLine.Command;
+import picocli.CommandLine.Option;
+
+import java.io.IOException;
+
+/**
+ * Handler to start container balancer.
+ */
+@Command(
+ name = "start",
+ description = "Start ContainerBalancer",
+ mixinStandardHelpOptions = true,
+ versionProvider = HddsVersionProvider.class)
+public class ContainerBalancerStartSubcommand extends ScmSubcommand {
+
+ @Option(names = {"-t", "--threshold"},
+ description = "Threshold target whether the cluster is balanced")
+ private double threshold;
+
+ @Option(names = {"-i", "--idleiterations"},
+ description = "Maximum consecutive idle iterations",
+ //idleiteration could be as follows:
+ // 0 ： indicate running infinitely
+ // positive number ： indicate the specific iterations
+ // -1 : user does not set this value
+ defaultValue = "-1")
+ private int idleiterations;
+
+ @Option(names = {"-d", "--maxDatanodesToBalance"},
+ description = "Maximum datanodes to move")
+ private int maxDatanodesToBalance;
+
+ @Option(names = {"-s", "--maxSizeToMove"},
+ description = "Maximum size to move")
+ private long maxSizeToMove;
+
+ @Override
+ public void execute(ScmClient scmClient) throws IOException {
+ boolean result = scmClient.startContainerBalancer(threshold, idleiterations,
+ maxDatanodesToBalance, maxSizeToMove);
+ if (result) {
+ System.out.println("Starting ContainerBalancer Successfully.");
+ return;
+ }
+ System.out.println("ContainerBalancer has been started" +
+ " by another cli command.");
Review comment:
Maybe we can print sth like "Container balancer is already running. Please stop it first."
##########
File path: hadoop-hdds/tools/src/main/java/org/apache/hadoop/hdds/scm/cli/ContainerBalancerStartSubcommand.java
##########
@@ -39,7 +39,12 @@
private double threshold;
@Option(names = {"-i", "--idleiterations"},
- description = "Maximum consecutive idle iterations")
+ description = "Maximum consecutive idle iterations",
+ //idleiteration could be as follows:
+ // 0 ： indicate running infinitely
+ // positive number ： indicate the specific iterations
+ // -1 : user does not set this value
+ defaultValue = "-1")
Review comment:
How about we use -1 as the value for running balancer infinitely? The possible value are >= -1 then. We don't need a default value.
Maybe internally in start balancer command it would be better to use Optional. So that you would know if user has specified an option like idleIteration 0. For input 0 I think the command should throw an exception because that is like not starting the balancer.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] siddhantsangwan commented on a change in pull request #2278: HDDS-4926. Support start/stop for container balancer via command line
Posted by GitBox <gi...@apache.org>.
siddhantsangwan commented on a change in pull request #2278:
URL: https://github.com/apache/ozone/pull/2278#discussion_r642277133
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/protocol/StorageContainerLocationProtocolServerSideTranslatorPB.java
##########
@@ -611,6 +638,28 @@ public ReplicationManagerStatusResponseProto getReplicationManagerStatus(
.setIsRunning(impl.getReplicationManagerStatus()).build();
}
+ public StartContainerBalancerResponseProto startContainerBalancer(
+ StartContainerBalancerRequestProto request)
+ throws IOException {
+ impl.startContainerBalancer();
+ return StartContainerBalancerResponseProto.newBuilder().build();
+ }
Review comment:
Yes, I think this will make managing configs easier.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi commented on a change in pull request #2278: HDDS-4926. Support start/stop for container balancer via command line
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on a change in pull request #2278:
URL: https://github.com/apache/ozone/pull/2278#discussion_r643624294
##########
File path: hadoop-hdds/interface-admin/src/main/proto/ScmAdminProtocol.proto
##########
@@ -445,6 +454,32 @@ message GetContainerTokenResponseProto {
required TokenProto token = 1;
}
+message StartContainerBalancerRequestProto {
+ optional string traceID = 1;
+ required double threshold = 2;
Review comment:
Do all these required fileds have default values? If so, they are optional, insteal of required.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi commented on a change in pull request #2278: HDDS-4926. Support start/stop for container balancer via command line
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on a change in pull request #2278:
URL: https://github.com/apache/ozone/pull/2278#discussion_r643620307
##########
File path: hadoop-hdds/interface-admin/src/main/proto/ScmAdminProtocol.proto
##########
@@ -445,6 +454,32 @@ message GetContainerTokenResponseProto {
required TokenProto token = 1;
}
+message StartContainerBalancerRequestProto {
+ optional string traceID = 1;
+ required double threshold = 2;
Review comment:
Can we using KeyValue here if there will be more fields in future.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi commented on a change in pull request #2278: HDDS-4926. Support start/stop for container balancer via command line
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on a change in pull request #2278:
URL: https://github.com/apache/ozone/pull/2278#discussion_r643617079
##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/protocol/StorageContainerLocationProtocol.java
##########
@@ -286,6 +286,24 @@ Pipeline createReplicationPipeline(HddsProtos.ReplicationType type,
*/
boolean getReplicationManagerStatus() throws IOException;
+ /**
+ * Start ContainerBalancer.
+ */
+ void startContainerBalancer(double threshold, int idleiterations,
+ int maxDatanodesToBalance, long maxSizeToMove) throws IOException;
Review comment:
same as above.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] JacksonYao287 commented on a change in pull request #2278: HDDS-4926. Support start/stop for container balancer via command line
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on a change in pull request #2278:
URL: https://github.com/apache/ozone/pull/2278#discussion_r651415720
##########
File path: hadoop-hdds/tools/src/main/java/org/apache/hadoop/hdds/scm/cli/ContainerBalancerStartSubcommand.java
##########
@@ -39,7 +39,12 @@
private double threshold;
@Option(names = {"-i", "--idleiterations"},
- description = "Maximum consecutive idle iterations")
+ description = "Maximum consecutive idle iterations",
+ //idleiteration could be as follows:
+ // 0 ： indicate running infinitely
+ // positive number ： indicate the specific iterations
+ // -1 : user does not set this value
+ defaultValue = "-1")
Review comment:
i think it would be a little complex to use optional here , since we just need to avoid an input idleiteration with 0 and this can be implemented by a simple judgement. what is more, optional is usually used to handle null(maybe not 0 or some other special values), so i think it is better not to use optional here, what do you think?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi commented on a change in pull request #2278: HDDS-4926. Support start/stop for container balancer via command line
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on a change in pull request #2278:
URL: https://github.com/apache/ozone/pull/2278#discussion_r656710599
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java
##########
@@ -96,42 +101,63 @@ public ContainerBalancer(
this.underUtilizedNodes = new ArrayList<>();
this.unBalancedNodes = new ArrayList<>();
this.withinThresholdUtilizedNodes = new ArrayList<>();
+ this.lock = new ReentrantLock();
}
-
/**
* Starts ContainerBalancer. Current implementation is incomplete.
*
* @param balancerConfiguration Configuration values.
*/
- public boolean start(ContainerBalancerConfiguration balancerConfiguration) {
- if (!balancerRunning.compareAndSet(false, true)) {
- LOG.error("Container Balancer is already running.");
- return false;
+ public boolean start(
+ ContainerBalancerConfiguration balancerConfiguration) {
+ lock.lock();
+ try {
+ if (!balancerRunning.compareAndSet(false, true)) {
+ LOG.error("Container Balancer is already running.");
Review comment:
error -> info
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] lokeshj1703 commented on a change in pull request #2278: HDDS-4926. Support start/stop for container balancer via command line
Posted by GitBox <gi...@apache.org>.
lokeshj1703 commented on a change in pull request #2278:
URL: https://github.com/apache/ozone/pull/2278#discussion_r655117955
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerConfiguration.java
##########
@@ -54,7 +54,12 @@
defaultValue = "10GB", tags = {ConfigTag.BALANCER},
description = "The maximum size of data in bytes that will be moved " +
"by Container Balancer.")
- private long maxSizeToMove = 10 * OzoneConsts.GB;
+ private long maxSizeToMoveInGB = 10 * OzoneConsts.GB;
Review comment:
We can keep this config same as before. I meant only changing the command line parameter name to maxSizeToMoveInGB. Sorry about the confusion!
Even for the command line I think we will be better off supporting a string representation. But we can do it in a separate PR. +1 o.w.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] JacksonYao287 commented on pull request #2278: HDDS-4926. Support start/stop for container balancer via command line
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on pull request #2278:
URL: https://github.com/apache/ozone/pull/2278#issuecomment-846773801
@lokeshj1703 PTAL
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi commented on a change in pull request #2278: HDDS-4926. Support start/stop for container balancer via command line
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on a change in pull request #2278:
URL: https://github.com/apache/ozone/pull/2278#discussion_r656709397
##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/protocol/StorageContainerLocationProtocol.java
##########
@@ -286,6 +287,26 @@ Pipeline createReplicationPipeline(HddsProtos.ReplicationType type,
*/
boolean getReplicationManagerStatus() throws IOException;
+ /**
+ * Start ContainerBalancer.
+ */
+ boolean startContainerBalancer(Optional<Double> threshold,
+ Optional<Integer> idleiterations,
+ Optional<Integer> maxDatanodesToBalance,
+ Optional<Long> maxSizeToMoveInGB) throws IOException;
Review comment:
Same as above.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] JacksonYao287 commented on pull request #2278: HDDS-4926. Support start/stop for container balancer via command line
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on pull request #2278:
URL: https://github.com/apache/ozone/pull/2278#issuecomment-853670178
@adoroszlai Hey，can you help having a look at this CI problem `TestReportPublisher.testCRLStatusReportPublisher`? it seems nothing to do with my PR, i do not modify any CRL related code.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] lokeshj1703 commented on pull request #2278: HDDS-4926. Support start/stop for container balancer via command line
Posted by GitBox <gi...@apache.org>.
lokeshj1703 commented on pull request #2278:
URL: https://github.com/apache/ozone/pull/2278#issuecomment-864915265
@GlenGeng @ChenSammi @siddhantsangwan Do you have any other comments?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] lokeshj1703 commented on a change in pull request #2278: HDDS-4926. Support start/stop for container balancer via command line
Posted by GitBox <gi...@apache.org>.
lokeshj1703 commented on a change in pull request #2278:
URL: https://github.com/apache/ozone/pull/2278#discussion_r642274979
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/protocol/StorageContainerLocationProtocolServerSideTranslatorPB.java
##########
@@ -611,6 +638,28 @@ public ReplicationManagerStatusResponseProto getReplicationManagerStatus(
.setIsRunning(impl.getReplicationManagerStatus()).build();
}
+ public StartContainerBalancerResponseProto startContainerBalancer(
+ StartContainerBalancerRequestProto request)
+ throws IOException {
+ impl.startContainerBalancer();
+ return StartContainerBalancerResponseProto.newBuilder().build();
+ }
Review comment:
What Siddhant is suggesting is that before calling ContainerBalancer#start, a ContainerBalancerConfiguration can be generated from the input parameters. If user passes parameter of threshold as 0.05, it can be set in the configuration object and then the conf object can be passed to balancer. No other change would be required in Balancer.
@siddhantsangwan Please correct me if I am wrong.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] JacksonYao287 commented on a change in pull request #2278: HDDS-4926. Support start/stop for container balancer via command line
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on a change in pull request #2278:
URL: https://github.com/apache/ozone/pull/2278#discussion_r652540989
##########
File path: hadoop-hdds/tools/src/main/java/org/apache/hadoop/hdds/scm/cli/ContainerBalancerStartSubcommand.java
##########
@@ -39,7 +39,12 @@
private double threshold;
@Option(names = {"-i", "--idleiterations"},
- description = "Maximum consecutive idle iterations")
+ description = "Maximum consecutive idle iterations",
+ //idleiteration could be as follows:
+ // 0 ： indicate running infinitely
+ // positive number ： indicate the specific iterations
+ // -1 : user does not set this value
+ defaultValue = "-1")
Review comment:
i have update this PR , in which i replaced all the parameters with optional , PATL!
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai commented on pull request #2278: HDDS-4926. Support start/stop for container balancer via command line
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on pull request #2278:
URL: https://github.com/apache/ozone/pull/2278#issuecomment-853676434
> Hey，can you help having a look at this CI problem `TestReportPublisher.testCRLStatusReportPublisher`? it seems nothing to do with my PR, i do not modify any CRL related code.
Yes, it's unrelated. #2302 fixes it. Please wait for that to be merged first.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] JacksonYao287 commented on a change in pull request #2278: HDDS-4926. Support start/stop for container balancer via command line
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on a change in pull request #2278:
URL: https://github.com/apache/ozone/pull/2278#discussion_r644530507
##########
File path: hadoop-hdds/interface-admin/src/main/proto/ScmAdminProtocol.proto
##########
@@ -445,6 +454,32 @@ message GetContainerTokenResponseProto {
required TokenProto token = 1;
}
+message StartContainerBalancerRequestProto {
+ optional string traceID = 1;
+ required double threshold = 2;
Review comment:
Thanks @ChenSammi for the reivew. there will not be default value, see [this discuss](https://github.com/apache/ozone/pull/2278#discussion_r640330813)
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] JacksonYao287 commented on a change in pull request #2278: HDDS-4926. Support start/stop for container balancer via command line
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on a change in pull request #2278:
URL: https://github.com/apache/ozone/pull/2278#discussion_r655134416
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerConfiguration.java
##########
@@ -54,7 +54,12 @@
defaultValue = "10GB", tags = {ConfigTag.BALANCER},
description = "The maximum size of data in bytes that will be moved " +
"by Container Balancer.")
- private long maxSizeToMove = 10 * OzoneConsts.GB;
+ private long maxSizeToMoveInGB = 10 * OzoneConsts.GB;
Review comment:
thanks for pointing out this , i will update the parameter.
>Even for the command line I think we will be better off supporting a string representation. But we can do it in a separate PR.
yes, i agree
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] JacksonYao287 commented on a change in pull request #2278: HDDS-4926. Support start/stop for container balancer via command line
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on a change in pull request #2278:
URL: https://github.com/apache/ozone/pull/2278#discussion_r651415885
##########
File path: hadoop-hdds/tools/src/main/java/org/apache/hadoop/hdds/scm/cli/ContainerBalancerStartSubcommand.java
##########
@@ -0,0 +1,69 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hdds.scm.cli;
+
+import org.apache.hadoop.hdds.cli.HddsVersionProvider;
+import org.apache.hadoop.hdds.scm.client.ScmClient;
+import picocli.CommandLine.Command;
+import picocli.CommandLine.Option;
+
+import java.io.IOException;
+
+/**
+ * Handler to start container balancer.
+ */
+@Command(
+ name = "start",
+ description = "Start ContainerBalancer",
+ mixinStandardHelpOptions = true,
+ versionProvider = HddsVersionProvider.class)
+public class ContainerBalancerStartSubcommand extends ScmSubcommand {
+
+ @Option(names = {"-t", "--threshold"},
+ description = "Threshold target whether the cluster is balanced")
+ private double threshold;
+
+ @Option(names = {"-i", "--idleiterations"},
+ description = "Maximum consecutive idle iterations",
+ //idleiteration could be as follows:
+ // 0 ： indicate running infinitely
+ // positive number ： indicate the specific iterations
+ // -1 : user does not set this value
+ defaultValue = "-1")
+ private int idleiterations;
+
+ @Option(names = {"-d", "--maxDatanodesToBalance"},
+ description = "Maximum datanodes to move")
+ private int maxDatanodesToBalance;
+
+ @Option(names = {"-s", "--maxSizeToMove"},
+ description = "Maximum size to move")
+ private long maxSizeToMove;
+
+ @Override
+ public void execute(ScmClient scmClient) throws IOException {
+ boolean result = scmClient.startContainerBalancer(threshold, idleiterations,
+ maxDatanodesToBalance, maxSizeToMove);
+ if (result) {
+ System.out.println("Starting ContainerBalancer Successfully.");
+ return;
+ }
+ System.out.println("ContainerBalancer has been started" +
+ " by another cli command.");
Review comment:
yes, thanks, i changed this , PATL!
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] siddhantsangwan commented on a change in pull request #2278: HDDS-4926. Support start/stop for container balancer via command line
Posted by GitBox <gi...@apache.org>.
siddhantsangwan commented on a change in pull request #2278:
URL: https://github.com/apache/ozone/pull/2278#discussion_r640357420
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/protocol/StorageContainerLocationProtocolServerSideTranslatorPB.java
##########
@@ -611,6 +638,28 @@ public ReplicationManagerStatusResponseProto getReplicationManagerStatus(
.setIsRunning(impl.getReplicationManagerStatus()).build();
}
+ public StartContainerBalancerResponseProto startContainerBalancer(
+ StartContainerBalancerRequestProto request)
+ throws IOException {
+ impl.startContainerBalancer();
+ return StartContainerBalancerResponseProto.newBuilder().build();
+ }
Review comment:
Can we pass the parameters in the request to `ContainerBalancerConfiguration`?
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java
##########
@@ -63,16 +68,14 @@ public ContainerBalancer(
/**
* Start ContainerBalancer. Current implementation is incomplete.
- *
- * @param balancerConfiguration Configuration values.
*/
- public void start(ContainerBalancerConfiguration balancerConfiguration) {
+ public void start() {
this.balancerRunning = true;
ozoneConfiguration = new OzoneConfiguration();
// initialise configs
- this.config = balancerConfiguration;
+ //this.config = balancerConfiguration;
Review comment:
`ContainerBalancerConfiguration` can be updated with any CLI params and then used as a single source of configurations wherever needed.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] lokeshj1703 commented on a change in pull request #2278: HDDS-4926. Support start/stop for container balancer via command line
Posted by GitBox <gi...@apache.org>.
lokeshj1703 commented on a change in pull request #2278:
URL: https://github.com/apache/ozone/pull/2278#discussion_r640333709
##########
File path: hadoop-hdds/tools/src/main/java/org/apache/hadoop/hdds/scm/cli/ContainerBalancerStartSubcommand.java
##########
@@ -0,0 +1,62 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hdds.scm.cli;
+
+import org.apache.hadoop.hdds.cli.HddsVersionProvider;
+import org.apache.hadoop.hdds.scm.client.ScmClient;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import picocli.CommandLine;
+import picocli.CommandLine.Command;
+
+import java.io.IOException;
+
+/**
+ * Handler to start container balancer.
+ */
+@Command(
+ name = "start",
+ description = "Start ContainerBalancer",
+ mixinStandardHelpOptions = true,
+ versionProvider = HddsVersionProvider.class)
+public class ContainerBalancerStartSubcommand extends ScmSubcommand {
+
+ private static final Logger LOG =
+ LoggerFactory.getLogger(ContainerBalancerStartSubcommand.class);
+
+ @CommandLine.Option(names = {"-t", "--threshold"},
+ description = "Threshold target whether the cluster is balanced",
+ defaultValue = "0.1",
Review comment:
Let's say user has set a configuration for threshold to be 0.05. If user does not specify threshold as parameter during start, threshold of 0.05 should be used in balancer rather than 0.1.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] lokeshj1703 commented on a change in pull request #2278: HDDS-4926. Support start/stop for container balancer via command line
Posted by GitBox <gi...@apache.org>.
lokeshj1703 commented on a change in pull request #2278:
URL: https://github.com/apache/ozone/pull/2278#discussion_r642271096
##########
File path: hadoop-hdds/tools/src/main/java/org/apache/hadoop/hdds/scm/cli/ContainerBalancerStartSubcommand.java
##########
@@ -0,0 +1,62 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hdds.scm.cli;
+
+import org.apache.hadoop.hdds.cli.HddsVersionProvider;
+import org.apache.hadoop.hdds.scm.client.ScmClient;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import picocli.CommandLine;
+import picocli.CommandLine.Command;
+
+import java.io.IOException;
+
+/**
+ * Handler to start container balancer.
+ */
+@Command(
+ name = "start",
+ description = "Start ContainerBalancer",
+ mixinStandardHelpOptions = true,
+ versionProvider = HddsVersionProvider.class)
+public class ContainerBalancerStartSubcommand extends ScmSubcommand {
+
+ private static final Logger LOG =
+ LoggerFactory.getLogger(ContainerBalancerStartSubcommand.class);
+
+ @CommandLine.Option(names = {"-t", "--threshold"},
+ description = "Threshold target whether the cluster is balanced",
+ defaultValue = "0.1",
Review comment:
It would be good to add a base test to test the CLI. For example simple test to start balancer and check status. Additional parameters when added can be tested in the UT as well.
Some UTs where it is done - TestContainerOperations, TestDecommissionSubCommand etc.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] JacksonYao287 commented on a change in pull request #2278: HDDS-4926. Support start/stop for container balancer via command line
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on a change in pull request #2278:
URL: https://github.com/apache/ozone/pull/2278#discussion_r644530507
##########
File path: hadoop-hdds/interface-admin/src/main/proto/ScmAdminProtocol.proto
##########
@@ -445,6 +454,32 @@ message GetContainerTokenResponseProto {
required TokenProto token = 1;
}
+message StartContainerBalancerRequestProto {
+ optional string traceID = 1;
+ required double threshold = 2;
Review comment:
Thanks @ChenSammi for the reivew. there will not be default value, see [this discuss](https://github.com/apache/ozone/pull/2278#discussion_r640330813)
##########
File path: hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/scm/protocolPB/StorageContainerLocationProtocolClientSideTranslatorPB.java
##########
@@ -712,6 +716,44 @@ public boolean getReplicationManagerStatus() throws IOException {
}
+ @Override
+ public void startContainerBalancer(double threshold, int idleiterations,
+ int maxDatanodesToBalance, long maxSizeToMove) throws IOException {
+
+ StartContainerBalancerRequestProto request =
+ StartContainerBalancerRequestProto.newBuilder()
+ .setIdleiterations(idleiterations)
+ .setMaxDatanodesToBalance(maxDatanodesToBalance)
+ .setMaxSizeToMove(maxSizeToMove)
+ .setThreshold(threshold).build();
+ submitRequest(Type.StartContainerBalancer,
+ builder -> builder.setStartContainerBalancerRequest(request));
+
+ }
+
+ @Override
+ public void stopContainerBalancer() throws IOException {
+
Review comment:
thanks , will remove it
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] JacksonYao287 commented on a change in pull request #2278: HDDS-4926. Support start/stop for container balancer via command line
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on a change in pull request #2278:
URL: https://github.com/apache/ozone/pull/2278#discussion_r645977843
##########
File path: hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/balancer/TestContainerBalancer.java
##########
@@ -96,11 +97,18 @@ public void setup() {
balancerConfiguration.setThreshold(randomThreshold);
containerBalancer.start(balancerConfiguration);
+
+ // waiting for balance completed.
+ // this is a temporary implementation for now
+ // modify this after balancer is fully completed
Review comment:
sure , will add this
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] JacksonYao287 commented on a change in pull request #2278: HDDS-4926. Support start/stop for container balancer via command line
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on a change in pull request #2278:
URL: https://github.com/apache/ozone/pull/2278#discussion_r643078435
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/protocol/StorageContainerLocationProtocolServerSideTranslatorPB.java
##########
@@ -611,6 +638,28 @@ public ReplicationManagerStatusResponseProto getReplicationManagerStatus(
.setIsRunning(impl.getReplicationManagerStatus()).build();
}
+ public StartContainerBalancerResponseProto startContainerBalancer(
+ StartContainerBalancerRequestProto request)
+ throws IOException {
+ impl.startContainerBalancer();
+ return StartContainerBalancerResponseProto.newBuilder().build();
+ }
Review comment:
updated! PATL @lokeshj1703 @siddhantsangwan @GlenGeng
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] JacksonYao287 commented on a change in pull request #2278: HDDS-4926. Support start/stop for container balancer via command line
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on a change in pull request #2278:
URL: https://github.com/apache/ozone/pull/2278#discussion_r639342133
##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/protocol/StorageContainerLocationProtocol.java
##########
@@ -286,6 +286,23 @@ Pipeline createReplicationPipeline(HddsProtos.ReplicationType type,
*/
boolean getReplicationManagerStatus() throws IOException;
+ /**
+ * Start ContainerBalancer.
+ */
+ void startContainerBalancer() throws IOException;
Review comment:
yes , i keep it like this , because for now we can not determine how these parameters will be used or should we add more parameters(like bandwidth, which determines the maximum speed at which a block will be moved from one datanode to another ). I just implement the simplest function for now, and i think it will not be hard to complete the full implementation of this after we review the design doc.
```
public void execute(ScmClient scmClient) throws IOException {
scmClient.startContainerBalancer();
LOG.info("Starting ContainerBalancer...");
//remove these in later implementation
LOG.info("threshold: {}", threshold);
LOG.info("idleiterations: {}", iCount);
}
```
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] JacksonYao287 commented on a change in pull request #2278: HDDS-4926. Support start/stop for container balancer via command line
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on a change in pull request #2278:
URL: https://github.com/apache/ozone/pull/2278#discussion_r656776512
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java
##########
@@ -670,6 +674,59 @@ public boolean getReplicationManagerStatus() {
return scm.getReplicationManager().isRunning();
}
+ @Override
+ public boolean startContainerBalancer(Optional<Double> threshold,
+ Optional<Integer> idleiterations,
+ Optional<Integer> maxDatanodesToBalance,
+ Optional<Long> maxSizeToMoveInGB) throws IOException{
+ getScm().checkAdminAccess(getRemoteUser());
+ AUDIT.logWriteSuccess(buildAuditMessageForSuccess(
Review comment:
yea, that make sense , i will do it
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi commented on a change in pull request #2278: HDDS-4926. Support start/stop for container balancer via command line
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on a change in pull request #2278:
URL: https://github.com/apache/ozone/pull/2278#discussion_r643616394
##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/client/ScmClient.java
##########
@@ -305,6 +305,24 @@ Pipeline createReplicationPipeline(HddsProtos.ReplicationType type,
*/
boolean getReplicationManagerStatus() throws IOException;
+ /**
+ * Start ContainerBalancer.
+ */
+ void startContainerBalancer(double threshold, int idleiterations,
Review comment:
Can we use K-V map to hold all parameters here, to avoid API change in future in case introducting new parameters
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] JacksonYao287 commented on a change in pull request #2278: HDDS-4926. Support start/stop for container balancer via command line
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on a change in pull request #2278:
URL: https://github.com/apache/ozone/pull/2278#discussion_r640553385
##########
File path: hadoop-hdds/tools/src/main/java/org/apache/hadoop/hdds/scm/cli/ContainerBalancerStartSubcommand.java
##########
@@ -0,0 +1,62 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hdds.scm.cli;
+
+import org.apache.hadoop.hdds.cli.HddsVersionProvider;
+import org.apache.hadoop.hdds.scm.client.ScmClient;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import picocli.CommandLine;
+import picocli.CommandLine.Command;
+
+import java.io.IOException;
+
+/**
+ * Handler to start container balancer.
+ */
+@Command(
+ name = "start",
+ description = "Start ContainerBalancer",
+ mixinStandardHelpOptions = true,
+ versionProvider = HddsVersionProvider.class)
+public class ContainerBalancerStartSubcommand extends ScmSubcommand {
+
+ private static final Logger LOG =
+ LoggerFactory.getLogger(ContainerBalancerStartSubcommand.class);
+
+ @CommandLine.Option(names = {"-t", "--threshold"},
+ description = "Threshold target whether the cluster is balanced",
+ defaultValue = "0.1",
Review comment:
updated, PTAL!
there is no any logic code here , just send a command , so maybe no need to add unit test , just like replication manager cli. what is more , acceptance test can be added after we complete container balancer, because by then the acceptance test for balancer cli will make more sense! what do you think about？
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi commented on a change in pull request #2278: HDDS-4926. Support start/stop for container balancer via command line
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on a change in pull request #2278:
URL: https://github.com/apache/ozone/pull/2278#discussion_r643617512
##########
File path: hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/scm/protocolPB/StorageContainerLocationProtocolClientSideTranslatorPB.java
##########
@@ -712,6 +716,44 @@ public boolean getReplicationManagerStatus() throws IOException {
}
+ @Override
+ public void startContainerBalancer(double threshold, int idleiterations,
+ int maxDatanodesToBalance, long maxSizeToMove) throws IOException {
+
+ StartContainerBalancerRequestProto request =
+ StartContainerBalancerRequestProto.newBuilder()
+ .setIdleiterations(idleiterations)
+ .setMaxDatanodesToBalance(maxDatanodesToBalance)
+ .setMaxSizeToMove(maxSizeToMove)
+ .setThreshold(threshold).build();
+ submitRequest(Type.StartContainerBalancer,
+ builder -> builder.setStartContainerBalancerRequest(request));
+
+ }
+
+ @Override
+ public void stopContainerBalancer() throws IOException {
+
Review comment:
Unnecessary blank line can be removed.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] JacksonYao287 commented on a change in pull request #2278: HDDS-4926. Support start/stop for container balancer via command line
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on a change in pull request #2278:
URL: https://github.com/apache/ozone/pull/2278#discussion_r642340854
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/protocol/StorageContainerLocationProtocolServerSideTranslatorPB.java
##########
@@ -611,6 +638,28 @@ public ReplicationManagerStatusResponseProto getReplicationManagerStatus(
.setIsRunning(impl.getReplicationManagerStatus()).build();
}
+ public StartContainerBalancerResponseProto startContainerBalancer(
+ StartContainerBalancerRequestProto request)
+ throws IOException {
+ impl.startContainerBalancer();
+ return StartContainerBalancerResponseProto.newBuilder().build();
+ }
Review comment:
i see the conversion [here](https://github.com/apache/ozone/pull/2230#discussion_r639555281), which mentioned that balance configuration will not be needed. should we add it back？ maybe we could keep it like this for now , and add it back if needed as we move forward with the implementation. what do you think @lokeshj1703 @siddhantsangwan
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] JacksonYao287 commented on a change in pull request #2278: HDDS-4926. Support start/stop for container balancer via command line
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on a change in pull request #2278:
URL: https://github.com/apache/ozone/pull/2278#discussion_r643078435
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/protocol/StorageContainerLocationProtocolServerSideTranslatorPB.java
##########
@@ -611,6 +638,28 @@ public ReplicationManagerStatusResponseProto getReplicationManagerStatus(
.setIsRunning(impl.getReplicationManagerStatus()).build();
}
+ public StartContainerBalancerResponseProto startContainerBalancer(
+ StartContainerBalancerRequestProto request)
+ throws IOException {
+ impl.startContainerBalancer();
+ return StartContainerBalancerResponseProto.newBuilder().build();
+ }
Review comment:
updated! PATL @lokeshj1703 @siddhantsangwan @GlenGeng
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] JacksonYao287 commented on a change in pull request #2278: HDDS-4926. Support start/stop for container balancer via command line
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on a change in pull request #2278:
URL: https://github.com/apache/ozone/pull/2278#discussion_r656784059
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerConfiguration.java
##########
@@ -56,6 +56,11 @@
"by Container Balancer.")
private long maxSizeToMove = 10 * OzoneConsts.GB;
+ @Config(key = "idle.iterations", type = ConfigType.INT,
Review comment:
yea, we could revisit it later
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] JacksonYao287 commented on a change in pull request #2278: HDDS-4926. Support start/stop for container balancer via command line
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on a change in pull request #2278:
URL: https://github.com/apache/ozone/pull/2278#discussion_r642324974
##########
File path: hadoop-hdds/tools/src/main/java/org/apache/hadoop/hdds/scm/cli/ContainerBalancerStartSubcommand.java
##########
@@ -0,0 +1,62 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hdds.scm.cli;
+
+import org.apache.hadoop.hdds.cli.HddsVersionProvider;
+import org.apache.hadoop.hdds.scm.client.ScmClient;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import picocli.CommandLine;
+import picocli.CommandLine.Command;
+
+import java.io.IOException;
+
+/**
+ * Handler to start container balancer.
+ */
+@Command(
+ name = "start",
+ description = "Start ContainerBalancer",
+ mixinStandardHelpOptions = true,
+ versionProvider = HddsVersionProvider.class)
+public class ContainerBalancerStartSubcommand extends ScmSubcommand {
+
+ private static final Logger LOG =
+ LoggerFactory.getLogger(ContainerBalancerStartSubcommand.class);
+
+ @CommandLine.Option(names = {"-t", "--threshold"},
+ description = "Threshold target whether the cluster is balanced",
+ defaultValue = "0.1",
Review comment:
ok, will add test
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] JacksonYao287 commented on a change in pull request #2278: HDDS-4926. Support start/stop for container balancer via command line
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on a change in pull request #2278:
URL: https://github.com/apache/ozone/pull/2278#discussion_r655134416
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerConfiguration.java
##########
@@ -54,7 +54,12 @@
defaultValue = "10GB", tags = {ConfigTag.BALANCER},
description = "The maximum size of data in bytes that will be moved " +
"by Container Balancer.")
- private long maxSizeToMove = 10 * OzoneConsts.GB;
+ private long maxSizeToMoveInGB = 10 * OzoneConsts.GB;
Review comment:
thanks for pointing out this , i will update the parameter.
>Even for the command line I think we will be better off supporting a string representation. But we can do it in a separate PR.
yes, i agree
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] JacksonYao287 commented on a change in pull request #2278: HDDS-4926. Support start/stop for container balancer via command line
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on a change in pull request #2278:
URL: https://github.com/apache/ozone/pull/2278#discussion_r646331943
##########
File path: hadoop-hdds/tools/src/main/java/org/apache/hadoop/hdds/scm/cli/ContainerBalancerStartSubcommand.java
##########
@@ -0,0 +1,64 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hdds.scm.cli;
+
+import org.apache.hadoop.hdds.cli.HddsVersionProvider;
+import org.apache.hadoop.hdds.scm.client.ScmClient;
+import picocli.CommandLine;
+import picocli.CommandLine.Command;
+
+import java.io.IOException;
+
+/**
+ * Handler to start container balancer.
+ */
+@Command(
+ name = "start",
+ description = "Start ContainerBalancer",
+ mixinStandardHelpOptions = true,
+ versionProvider = HddsVersionProvider.class)
+public class ContainerBalancerStartSubcommand extends ScmSubcommand {
+
+ @CommandLine.Option(names = {"-t", "--threshold"},
+ description = "Threshold target whether the cluster is balanced")
+ private double threshold;
Review comment:
thanks @lokeshj1703 for pointing out this! but ,according to my experience , it is not a good choice to use Optional as parameter, see [Java static code analysis](https://rules.sonarsource.com/java/RSPEC-3553), so i use another approach to make it optional, and add some checks to validate the input parameters. please take a look and correct me if i misunderstand.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] JacksonYao287 commented on a change in pull request #2278: HDDS-4926. Support start/stop for container balancer via command line
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on a change in pull request #2278:
URL: https://github.com/apache/ozone/pull/2278#discussion_r646257935
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java
##########
@@ -323,8 +344,22 @@ public static double calculateUtilization(
* Stops ContainerBalancer.
*/
public void stop() {
Review comment:
thanks @lokeshj1703 for point out this!
>In a race condition it is possible for balancerRunning to be set as true but thread may not be running
sorry, i can not find a condition like this. but i can find out a race condition that balancerRunning has been set to false , but the balance thread is still running.
e.g. , `balancer#start` is triggered by cli and `balanceRunning` is atomically set to true, but the balance thread is not been started , at this time , `balancer#stop` is triggered by cli, and `balanceRunning` is atomically set to false.then `balancer#start` continue to execute , and a balance thread will be created and go on. so in this case , `balancerRunning` has been set to false , but the balance thread is still running. please correct if i am wrong!
but , in any case , start and stop should be synchronized. i will do this in a update commit
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi commented on a change in pull request #2278: HDDS-4926. Support start/stop for container balancer via command line
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on a change in pull request #2278:
URL: https://github.com/apache/ozone/pull/2278#discussion_r656709307
##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/client/ScmClient.java
##########
@@ -305,6 +306,26 @@ Pipeline createReplicationPipeline(HddsProtos.ReplicationType type,
*/
boolean getReplicationManagerStatus() throws IOException;
+ /**
+ * Start ContainerBalancer.
+ */
+ boolean startContainerBalancer(Optional<Double> threshold,
+ Optional<Integer> idleiterations,
Review comment:
We use 4 black space indent when a line wraps.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] JacksonYao287 commented on a change in pull request #2278: HDDS-4926. Support start/stop for container balancer via command line
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on a change in pull request #2278:
URL: https://github.com/apache/ozone/pull/2278#discussion_r639342133
##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/protocol/StorageContainerLocationProtocol.java
##########
@@ -286,6 +286,23 @@ Pipeline createReplicationPipeline(HddsProtos.ReplicationType type,
*/
boolean getReplicationManagerStatus() throws IOException;
+ /**
+ * Start ContainerBalancer.
+ */
+ void startContainerBalancer() throws IOException;
Review comment:
yes , i keep it like this , because for now we can not determine how these parameters will be used or should we add more parameters(like bandwidth, which determines the maximum speed at which a block will be moved from one datanode to another ). I just implement the simplest function for now, and i think it will not be hard to complete the full implementation of this after we review the design doc and make the final decision.
```
public void execute(ScmClient scmClient) throws IOException {
scmClient.startContainerBalancer();
LOG.info("Starting ContainerBalancer...");
//remove these in later implementation
LOG.info("threshold: {}", threshold);
LOG.info("idleiterations: {}", iCount);
}
```
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] JacksonYao287 commented on a change in pull request #2278: HDDS-4926. Support start/stop for container balancer via command line
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on a change in pull request #2278:
URL: https://github.com/apache/ozone/pull/2278#discussion_r642360725
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/protocol/StorageContainerLocationProtocolServerSideTranslatorPB.java
##########
@@ -611,6 +638,28 @@ public ReplicationManagerStatusResponseProto getReplicationManagerStatus(
.setIsRunning(impl.getReplicationManagerStatus()).build();
}
+ public StartContainerBalancerResponseProto startContainerBalancer(
+ StartContainerBalancerRequestProto request)
+ throws IOException {
+ impl.startContainerBalancer();
+ return StartContainerBalancerResponseProto.newBuilder().build();
+ }
Review comment:
thanks , it seems good , i will add it back , as well as some tests
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] lokeshj1703 commented on a change in pull request #2278: HDDS-4926. Support start/stop for container balancer via command line
Posted by GitBox <gi...@apache.org>.
lokeshj1703 commented on a change in pull request #2278:
URL: https://github.com/apache/ozone/pull/2278#discussion_r646417633
##########
File path: hadoop-hdds/tools/src/main/java/org/apache/hadoop/hdds/scm/cli/ContainerBalancerStartSubcommand.java
##########
@@ -0,0 +1,64 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hdds.scm.cli;
+
+import org.apache.hadoop.hdds.cli.HddsVersionProvider;
+import org.apache.hadoop.hdds.scm.client.ScmClient;
+import picocli.CommandLine;
+import picocli.CommandLine.Command;
+
+import java.io.IOException;
+
+/**
+ * Handler to start container balancer.
+ */
+@Command(
+ name = "start",
+ description = "Start ContainerBalancer",
+ mixinStandardHelpOptions = true,
+ versionProvider = HddsVersionProvider.class)
+public class ContainerBalancerStartSubcommand extends ScmSubcommand {
+
+ @CommandLine.Option(names = {"-t", "--threshold"},
+ description = "Threshold target whether the cluster is balanced")
+ private double threshold;
Review comment:
Sure, this works too.
Optional would work in our case because we are mostly passing these params if present to a protobuf or a configuration object. It might make code simpler IMO. But I am fine with the above approach too.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] JacksonYao287 commented on a change in pull request #2278: HDDS-4926. Support start/stop for container balancer via command line
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on a change in pull request #2278:
URL: https://github.com/apache/ozone/pull/2278#discussion_r648197219
##########
File path: hadoop-hdds/tools/src/main/java/org/apache/hadoop/hdds/scm/cli/ContainerBalancerCommands.java
##########
@@ -0,0 +1,101 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hdds.scm.cli;
+
+import java.util.concurrent.Callable;
+
+import org.apache.hadoop.hdds.cli.GenericCli;
+import org.apache.hadoop.hdds.cli.HddsVersionProvider;
+import org.apache.hadoop.hdds.cli.OzoneAdmin;
+import org.apache.hadoop.hdds.cli.SubcommandWithParent;
+
+import org.kohsuke.MetaInfServices;
+import picocli.CommandLine.Command;
+import picocli.CommandLine.Model.CommandSpec;
+import picocli.CommandLine.Spec;
+
+/**
+ * Subcommand to group container balancer related operations.
+ *
+ * <p>The balancer is a tool that balances datanode space usage on an Ozone
+ * cluster when some datanodes become full or when new empty nodes join
+ * the cluster. The tool can be run by the cluster administrator
+ * from command line while applications adding and deleting blocks.
+ *
+ * <p>SYNOPSIS
+ * <pre>
+ * To start:
+ * ozone admin containerbalancer start
+ * [ -t/--threshold {@literal <threshold>}]
+ * [ -i/--idleiterations {@literal <idleiterations>}]
+ * Example: ozone admin containerbalancer start
+ * start balancer with a default threshold of 10%
+ * ozone admin containerbalancer start -t 5
+ * start balancer with a threshold of 5%
+ * ozone admin containerbalancer start -i 20
+ * start balancer with maximum 20 consecutive idle iterations
+ * ozone admin containerbalancer start -i -1
+ * run balancer with default threshold infinitely
Review comment:
thanks @lokeshj1703 for pointing out this, will update it!
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] lokeshj1703 commented on a change in pull request #2278: HDDS-4926. Support start/stop for container balancer via command line
Posted by GitBox <gi...@apache.org>.
lokeshj1703 commented on a change in pull request #2278:
URL: https://github.com/apache/ozone/pull/2278#discussion_r651437176
##########
File path: hadoop-hdds/tools/src/main/java/org/apache/hadoop/hdds/scm/cli/ContainerBalancerStartSubcommand.java
##########
@@ -39,7 +39,12 @@
private double threshold;
@Option(names = {"-i", "--idleiterations"},
- description = "Maximum consecutive idle iterations")
+ description = "Maximum consecutive idle iterations",
+ //idleiteration could be as follows:
+ // 0 ： indicate running infinitely
+ // positive number ： indicate the specific iterations
+ // -1 : user does not set this value
+ defaultValue = "-1")
Review comment:
How do you know user has entered 0? The variable will be initialised as 0 always.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] JacksonYao287 commented on a change in pull request #2278: HDDS-4926. Support start/stop for container balancer via command line
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on a change in pull request #2278:
URL: https://github.com/apache/ozone/pull/2278#discussion_r639342133
##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/protocol/StorageContainerLocationProtocol.java
##########
@@ -286,6 +286,23 @@ Pipeline createReplicationPipeline(HddsProtos.ReplicationType type,
*/
boolean getReplicationManagerStatus() throws IOException;
+ /**
+ * Start ContainerBalancer.
+ */
+ void startContainerBalancer() throws IOException;
Review comment:
yes , i keep it like this , because for now we can not determine how these parameters will be used or should we add more parameters(like bandwidth, which determines the maximum speed at which a block will be moved from one datanode to another ). I just implement the simplest function for now, and i think it will not be hard to complete the full implementation of this after we review the design doc.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] lokeshj1703 commented on a change in pull request #2278: HDDS-4926. Support start/stop for container balancer via command line
Posted by GitBox <gi...@apache.org>.
lokeshj1703 commented on a change in pull request #2278:
URL: https://github.com/apache/ozone/pull/2278#discussion_r640330813
##########
File path: hadoop-hdds/tools/src/main/java/org/apache/hadoop/hdds/scm/cli/ContainerBalancerStartSubcommand.java
##########
@@ -0,0 +1,62 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hdds.scm.cli;
+
+import org.apache.hadoop.hdds.cli.HddsVersionProvider;
+import org.apache.hadoop.hdds.scm.client.ScmClient;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import picocli.CommandLine;
+import picocli.CommandLine.Command;
+
+import java.io.IOException;
+
+/**
+ * Handler to start container balancer.
+ */
+@Command(
+ name = "start",
+ description = "Start ContainerBalancer",
+ mixinStandardHelpOptions = true,
+ versionProvider = HddsVersionProvider.class)
+public class ContainerBalancerStartSubcommand extends ScmSubcommand {
+
+ private static final Logger LOG =
+ LoggerFactory.getLogger(ContainerBalancerStartSubcommand.class);
+
+ @CommandLine.Option(names = {"-t", "--threshold"},
+ description = "Threshold target whether the cluster is balanced",
+ defaultValue = "0.1",
Review comment:
There would also be a corresponding configuration for threshold and other parameters. How do we make sure that the configuration defaults are in sync with the default values in command line?
I think it might be better to not include a default value here or have an invalid value by default.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] JacksonYao287 commented on pull request #2278: HDDS-4926. Support start/stop for container balancer via command line
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on pull request #2278:
URL: https://github.com/apache/ozone/pull/2278#issuecomment-853670178
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi commented on a change in pull request #2278: HDDS-4926. Support start/stop for container balancer via command line
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on a change in pull request #2278:
URL: https://github.com/apache/ozone/pull/2278#discussion_r656771350
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java
##########
@@ -670,6 +674,59 @@ public boolean getReplicationManagerStatus() {
return scm.getReplicationManager().isRunning();
}
+ @Override
+ public boolean startContainerBalancer(Optional<Double> threshold,
+ Optional<Integer> idleiterations,
+ Optional<Integer> maxDatanodesToBalance,
+ Optional<Long> maxSizeToMoveInGB) throws IOException{
+ getScm().checkAdminAccess(getRemoteUser());
+ AUDIT.logWriteSuccess(buildAuditMessageForSuccess(
Review comment:
I think we should write the audit log after the balancer is started at the end.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] JacksonYao287 commented on a change in pull request #2278: HDDS-4926. Support start/stop for container balancer via command line
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on a change in pull request #2278:
URL: https://github.com/apache/ozone/pull/2278#discussion_r650399462
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java
##########
@@ -675,17 +676,21 @@ public boolean startContainerBalancer(double threshold, int idleiterations,
AUDIT.logWriteSuccess(buildAuditMessageForSuccess(
SCMAction.START_CONTAINER_BALANCER, null));
ContainerBalancerConfiguration cbc = new ContainerBalancerConfiguration();
+
if (threshold > 0) {
cbc.setThreshold(threshold);
}
if (maxDatanodesToBalance > 0) {
cbc.setMaxDatanodesToBalance(maxDatanodesToBalance);
}
if (maxSizeToMove > 0) {
- cbc.setMaxSizeToMove(maxSizeToMove);
+ cbc.setMaxSizeToMove(maxSizeToMove * OzoneConsts.GB);
Review comment:
yea, this sounds good, will update this
##########
File path: hadoop-hdds/tools/src/main/java/org/apache/hadoop/hdds/scm/cli/ContainerBalancerStartSubcommand.java
##########
@@ -39,7 +39,12 @@
private double threshold;
@Option(names = {"-i", "--idleiterations"},
- description = "Maximum consecutive idle iterations")
+ description = "Maximum consecutive idle iterations",
+ //idleiteration could be as follows:
+ // 0 ： indicate running infinitely
+ // positive number ： indicate the specific iterations
+ // -1 : user does not set this value
+ defaultValue = "-1")
Review comment:
yes , this seems better , will change this
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] JacksonYao287 commented on a change in pull request #2278: HDDS-4926. Support start/stop for container balancer via command line
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on a change in pull request #2278:
URL: https://github.com/apache/ozone/pull/2278#discussion_r640528380
##########
File path: hadoop-hdds/tools/src/main/java/org/apache/hadoop/hdds/scm/cli/ContainerBalancerStartSubcommand.java
##########
@@ -0,0 +1,62 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hdds.scm.cli;
+
+import org.apache.hadoop.hdds.cli.HddsVersionProvider;
+import org.apache.hadoop.hdds.scm.client.ScmClient;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import picocli.CommandLine;
+import picocli.CommandLine.Command;
+
+import java.io.IOException;
+
+/**
+ * Handler to start container balancer.
+ */
+@Command(
+ name = "start",
+ description = "Start ContainerBalancer",
+ mixinStandardHelpOptions = true,
+ versionProvider = HddsVersionProvider.class)
+public class ContainerBalancerStartSubcommand extends ScmSubcommand {
+
+ private static final Logger LOG =
+ LoggerFactory.getLogger(ContainerBalancerStartSubcommand.class);
+
+ @CommandLine.Option(names = {"-t", "--threshold"},
+ description = "Threshold target whether the cluster is balanced",
+ defaultValue = "0.1",
Review comment:
thanks @lokeshj1703 for the review ! will remove the default value.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] lokeshj1703 commented on pull request #2278: HDDS-4926. Support start/stop for container balancer via command line
Posted by GitBox <gi...@apache.org>.
lokeshj1703 commented on pull request #2278:
URL: https://github.com/apache/ozone/pull/2278#issuecomment-864915265
@GlenGeng @ChenSammi @siddhantsangwan Do you have any other comments?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] GlenGeng commented on a change in pull request #2278: HDDS-4926. Support start/stop for container balancer via command line
Posted by GitBox <gi...@apache.org>.
GlenGeng commented on a change in pull request #2278:
URL: https://github.com/apache/ozone/pull/2278#discussion_r637768632
##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/protocol/StorageContainerLocationProtocol.java
##########
@@ -286,6 +286,23 @@ Pipeline createReplicationPipeline(HddsProtos.ReplicationType type,
*/
boolean getReplicationManagerStatus() throws IOException;
+ /**
+ * Start ContainerBalancer.
+ */
+ void startContainerBalancer() throws IOException;
Review comment:
According to our design decision, should we add the parameters like `threshold` and `consecutiveIdleIterations` in the start ?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi commented on a change in pull request #2278: HDDS-4926. Support start/stop for container balancer via command line
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on a change in pull request #2278:
URL: https://github.com/apache/ozone/pull/2278#discussion_r656710916
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java
##########
@@ -96,42 +101,63 @@ public ContainerBalancer(
this.underUtilizedNodes = new ArrayList<>();
this.unBalancedNodes = new ArrayList<>();
this.withinThresholdUtilizedNodes = new ArrayList<>();
+ this.lock = new ReentrantLock();
}
-
/**
* Starts ContainerBalancer. Current implementation is incomplete.
*
* @param balancerConfiguration Configuration values.
*/
- public boolean start(ContainerBalancerConfiguration balancerConfiguration) {
- if (!balancerRunning.compareAndSet(false, true)) {
- LOG.error("Container Balancer is already running.");
- return false;
+ public boolean start(
+ ContainerBalancerConfiguration balancerConfiguration) {
+ lock.lock();
+ try {
+ if (!balancerRunning.compareAndSet(false, true)) {
+ LOG.error("Container Balancer is already running.");
+ return false;
+ }
+
+ ozoneConfiguration = new OzoneConfiguration();
Review comment:
ozoneConfiguration seems not used later.
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java
##########
@@ -96,42 +101,63 @@ public ContainerBalancer(
this.underUtilizedNodes = new ArrayList<>();
this.unBalancedNodes = new ArrayList<>();
this.withinThresholdUtilizedNodes = new ArrayList<>();
+ this.lock = new ReentrantLock();
}
-
/**
* Starts ContainerBalancer. Current implementation is incomplete.
*
* @param balancerConfiguration Configuration values.
*/
- public boolean start(ContainerBalancerConfiguration balancerConfiguration) {
- if (!balancerRunning.compareAndSet(false, true)) {
- LOG.error("Container Balancer is already running.");
- return false;
+ public boolean start(
+ ContainerBalancerConfiguration balancerConfiguration) {
+ lock.lock();
+ try {
+ if (!balancerRunning.compareAndSet(false, true)) {
+ LOG.error("Container Balancer is already running.");
+ return false;
+ }
+
+ ozoneConfiguration = new OzoneConfiguration();
Review comment:
ozoneConfiguration seems not be used later.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi commented on a change in pull request #2278: HDDS-4926. Support start/stop for container balancer via command line
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on a change in pull request #2278:
URL: https://github.com/apache/ozone/pull/2278#discussion_r656710316
##########
File path: hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/scm/protocolPB/StorageContainerLocationProtocolClientSideTranslatorPB.java
##########
@@ -712,6 +718,73 @@ public boolean getReplicationManagerStatus() throws IOException {
}
+ @Override
+ public boolean startContainerBalancer(Optional<Double> threshold,
+ Optional<Integer> idleiterations,
+ Optional<Integer> maxDatanodesToBalance,
+ Optional<Long> maxSizeToMoveInGB) throws IOException{
+ StartContainerBalancerRequestProto.Builder builder =
+ StartContainerBalancerRequestProto.newBuilder();
+ builder.setTraceID(TracingUtil.exportCurrentSpan());
+
+ //make balancer configuration optional
+ if (threshold.isPresent()) {
+ double tsd = threshold.get();
+ Preconditions.checkState(tsd >= 0.0D && tsd < 1.0D,
+ "threshold should to be specified in range [0.0, 1.0).");
+ builder.setThreshold(tsd);
+ }
+ if (maxSizeToMoveInGB.isPresent()) {
+ long mstm = maxSizeToMoveInGB.get();
+ Preconditions.checkState(mstm > 0,
+ "maxSizeToMoveInGB must be positive.");
+ builder.setMaxSizeToMoveInGB(mstm);
+ }
+ if (maxDatanodesToBalance.isPresent()) {
+ int mdtb = maxDatanodesToBalance.get();
+ Preconditions.checkState(mdtb > 0,
+ "maxDatanodesToBalance must be positive.");
+ builder.setMaxDatanodesToBalance(mdtb);
+ }
+ if (idleiterations.isPresent()) {
+ int idi = idleiterations.get();
+ Preconditions.checkState(idi > 0 || idi == -1,
+ "maxDatanodesToBalance must be positive or" +
+ " -1(infinitly run container balancer).");
+ builder.setIdleiterations(idi);
+ }
+
+ StartContainerBalancerRequestProto request = builder.build();
+ StartContainerBalancerResponseProto response =
+ submitRequest(Type.StartContainerBalancer,
+ builder1 -> builder1.setStartContainerBalancerRequest(request))
+ .getStartContainerBalancerResponse();
+ return response.getStart();
+ }
+
+ @Override
+ public void stopContainerBalancer() throws IOException {
+
+ StopContainerBalancerRequestProto request =
+ StopContainerBalancerRequestProto.getDefaultInstance();
+ submitRequest(Type.StopContainerBalancer,
+ builder -> builder.setStopContainerBalancerRequest(request));
+
+ }
+
+ @Override
+ public boolean getContainerBalancerStatus() throws IOException {
+
+ ContainerBalancerStatusRequestProto request =
Review comment:
Same as above.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi commented on pull request #2278: HDDS-4926. Support start/stop for container balancer via command line
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on pull request #2278:
URL: https://github.com/apache/ozone/pull/2278#issuecomment-866760509
The last patch LGTM, +1. Thanks @JacksonYao287 for the contribution and @lokeshj1703 @siddhantsangwan for code review.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] lokeshj1703 commented on a change in pull request #2278: HDDS-4926. Support start/stop for container balancer via command line
Posted by GitBox <gi...@apache.org>.
lokeshj1703 commented on a change in pull request #2278:
URL: https://github.com/apache/ozone/pull/2278#discussion_r642348645
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/protocol/StorageContainerLocationProtocolServerSideTranslatorPB.java
##########
@@ -611,6 +638,28 @@ public ReplicationManagerStatusResponseProto getReplicationManagerStatus(
.setIsRunning(impl.getReplicationManagerStatus()).build();
}
+ public StartContainerBalancerResponseProto startContainerBalancer(
+ StartContainerBalancerRequestProto request)
+ throws IOException {
+ impl.startContainerBalancer();
+ return StartContainerBalancerResponseProto.newBuilder().build();
+ }
Review comment:
@JacksonYao287 ContainerBalancerConfiguration configuration would be needed. That is where all balancer related ozone configs are added. We should definitely add it back.
I think the conversation in #2230 was around having configuration as part of ContainerBalancer#start function. As @siddhantsangwan mentioned it would be good to have it there.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] JacksonYao287 commented on a change in pull request #2278: HDDS-4926. Support start/stop for container balancer via command line
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on a change in pull request #2278:
URL: https://github.com/apache/ozone/pull/2278#discussion_r637779725
##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/protocol/StorageContainerLocationProtocol.java
##########
@@ -286,6 +286,23 @@ Pipeline createReplicationPipeline(HddsProtos.ReplicationType type,
*/
boolean getReplicationManagerStatus() throws IOException;
+ /**
+ * Start ContainerBalancer.
+ */
+ void startContainerBalancer() throws IOException;
Review comment:
@GlenGeng Thanks for the review! will add these and some usage info.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] JacksonYao287 commented on a change in pull request #2278: HDDS-4926. Support start/stop for container balancer via command line
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on a change in pull request #2278:
URL: https://github.com/apache/ozone/pull/2278#discussion_r637841262
##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/protocol/StorageContainerLocationProtocol.java
##########
@@ -286,6 +286,23 @@ Pipeline createReplicationPipeline(HddsProtos.ReplicationType type,
*/
boolean getReplicationManagerStatus() throws IOException;
+ /**
+ * Start ContainerBalancer.
+ */
+ void startContainerBalancer() throws IOException;
Review comment:
updated. PTAL
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi commented on a change in pull request #2278: HDDS-4926. Support start/stop for container balancer via command line
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on a change in pull request #2278:
URL: https://github.com/apache/ozone/pull/2278#discussion_r656770614
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerConfiguration.java
##########
@@ -56,6 +56,11 @@
"by Container Balancer.")
private long maxSizeToMove = 10 * OzoneConsts.GB;
+ @Config(key = "idle.iterations", type = ConfigType.INT,
Review comment:
Based on current implementation, I would suggest changing the "idle.iterations" to "max.iterations". As Lokesh said, we can do it in a seperate PR later to revisit all these configurations and namings.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] JacksonYao287 commented on a change in pull request #2278: HDDS-4926. Support start/stop for container balancer via command line
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on a change in pull request #2278:
URL: https://github.com/apache/ozone/pull/2278#discussion_r656721960
##########
File path: hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/scm/protocolPB/StorageContainerLocationProtocolClientSideTranslatorPB.java
##########
@@ -712,6 +718,73 @@ public boolean getReplicationManagerStatus() throws IOException {
}
+ @Override
+ public boolean startContainerBalancer(Optional<Double> threshold,
+ Optional<Integer> idleiterations,
+ Optional<Integer> maxDatanodesToBalance,
+ Optional<Long> maxSizeToMoveInGB) throws IOException{
+ StartContainerBalancerRequestProto.Builder builder =
+ StartContainerBalancerRequestProto.newBuilder();
+ builder.setTraceID(TracingUtil.exportCurrentSpan());
+
+ //make balancer configuration optional
+ if (threshold.isPresent()) {
+ double tsd = threshold.get();
+ Preconditions.checkState(tsd >= 0.0D && tsd < 1.0D,
+ "threshold should to be specified in range [0.0, 1.0).");
+ builder.setThreshold(tsd);
+ }
+ if (maxSizeToMoveInGB.isPresent()) {
+ long mstm = maxSizeToMoveInGB.get();
+ Preconditions.checkState(mstm > 0,
+ "maxSizeToMoveInGB must be positive.");
+ builder.setMaxSizeToMoveInGB(mstm);
+ }
+ if (maxDatanodesToBalance.isPresent()) {
+ int mdtb = maxDatanodesToBalance.get();
+ Preconditions.checkState(mdtb > 0,
+ "maxDatanodesToBalance must be positive.");
+ builder.setMaxDatanodesToBalance(mdtb);
+ }
+ if (idleiterations.isPresent()) {
+ int idi = idleiterations.get();
+ Preconditions.checkState(idi > 0 || idi == -1,
+ "maxDatanodesToBalance must be positive or" +
+ " -1(infinitly run container balancer).");
+ builder.setIdleiterations(idi);
+ }
+
+ StartContainerBalancerRequestProto request = builder.build();
+ StartContainerBalancerResponseProto response =
+ submitRequest(Type.StartContainerBalancer,
+ builder1 -> builder1.setStartContainerBalancerRequest(request))
+ .getStartContainerBalancerResponse();
+ return response.getStart();
+ }
+
+ @Override
+ public void stopContainerBalancer() throws IOException {
+
+ StopContainerBalancerRequestProto request =
+ StopContainerBalancerRequestProto.getDefaultInstance();
+ submitRequest(Type.StopContainerBalancer,
+ builder -> builder.setStopContainerBalancerRequest(request));
+
+ }
+
+ @Override
+ public boolean getContainerBalancerStatus() throws IOException {
+
+ ContainerBalancerStatusRequestProto request =
Review comment:
seems i have already used 4 black space indent here
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] JacksonYao287 commented on pull request #2278: HDDS-4926. Support start/stop for container balancer via command line
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on pull request #2278:
URL: https://github.com/apache/ozone/pull/2278#issuecomment-866584509
Thank @siddhantsangwan for the review, i have fixed the comment, PTAL!
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi commented on a change in pull request #2278: HDDS-4926. Support start/stop for container balancer via command line
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on a change in pull request #2278:
URL: https://github.com/apache/ozone/pull/2278#discussion_r656709812
##########
File path: hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/scm/protocolPB/StorageContainerLocationProtocolClientSideTranslatorPB.java
##########
@@ -712,6 +716,44 @@ public boolean getReplicationManagerStatus() throws IOException {
}
+ @Override
+ public void startContainerBalancer(double threshold, int idleiterations,
+ int maxDatanodesToBalance, long maxSizeToMove) throws IOException {
+
+ StartContainerBalancerRequestProto request =
+ StartContainerBalancerRequestProto.newBuilder()
+ .setIdleiterations(idleiterations)
+ .setMaxDatanodesToBalance(maxDatanodesToBalance)
+ .setMaxSizeToMove(maxSizeToMove)
+ .setThreshold(threshold).build();
+ submitRequest(Type.StartContainerBalancer,
+ builder -> builder.setStartContainerBalancerRequest(request));
+
+ }
+
+ @Override
+ public void stopContainerBalancer() throws IOException {
+
Review comment:
Can we remove these extra head and tail black line?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] JacksonYao287 commented on pull request #2278: HDDS-4926. Support start/stop for container balancer via command line
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on pull request #2278:
URL: https://github.com/apache/ozone/pull/2278#issuecomment-866483100
thanks @ChenSammi for the review, i have fix the comment.PTAL!
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] JacksonYao287 commented on a change in pull request #2278: HDDS-4926. Support start/stop for container balancer via command line
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on a change in pull request #2278:
URL: https://github.com/apache/ozone/pull/2278#discussion_r644530729
##########
File path: hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/scm/protocolPB/StorageContainerLocationProtocolClientSideTranslatorPB.java
##########
@@ -712,6 +716,44 @@ public boolean getReplicationManagerStatus() throws IOException {
}
+ @Override
+ public void startContainerBalancer(double threshold, int idleiterations,
+ int maxDatanodesToBalance, long maxSizeToMove) throws IOException {
+
+ StartContainerBalancerRequestProto request =
+ StartContainerBalancerRequestProto.newBuilder()
+ .setIdleiterations(idleiterations)
+ .setMaxDatanodesToBalance(maxDatanodesToBalance)
+ .setMaxSizeToMove(maxSizeToMove)
+ .setThreshold(threshold).build();
+ submitRequest(Type.StartContainerBalancer,
+ builder -> builder.setStartContainerBalancerRequest(request));
+
+ }
+
+ @Override
+ public void stopContainerBalancer() throws IOException {
+
Review comment:
thanks , will remove it
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi merged pull request #2278: HDDS-4926. Support start/stop for container balancer via command line
Posted by GitBox <gi...@apache.org>.
ChenSammi merged pull request #2278:
URL: https://github.com/apache/ozone/pull/2278
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] lokeshj1703 commented on a change in pull request #2278: HDDS-4926. Support start/stop for container balancer via command line
Posted by GitBox <gi...@apache.org>.
lokeshj1703 commented on a change in pull request #2278:
URL: https://github.com/apache/ozone/pull/2278#discussion_r655117955
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerConfiguration.java
##########
@@ -54,7 +54,12 @@
defaultValue = "10GB", tags = {ConfigTag.BALANCER},
description = "The maximum size of data in bytes that will be moved " +
"by Container Balancer.")
- private long maxSizeToMove = 10 * OzoneConsts.GB;
+ private long maxSizeToMoveInGB = 10 * OzoneConsts.GB;
Review comment:
We can keep this config same as before. I meant only changing the command line parameter name to maxSizeToMoveInGB. Sorry about the confusion!
Even for the command line I think we will be better off supporting a string representation. But we can do it in a separate PR. +1 o.w.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] JacksonYao287 commented on a change in pull request #2278: HDDS-4926. Support start/stop for container balancer via command line
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on a change in pull request #2278:
URL: https://github.com/apache/ozone/pull/2278#discussion_r646257935
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java
##########
@@ -323,8 +344,22 @@ public static double calculateUtilization(
* Stops ContainerBalancer.
*/
public void stop() {
Review comment:
thanks @lokeshj1703 for point out this!
>In a race condition it is possible for balancerRunning to be set as true but thread may not be running
sorry, i can not find a condition like this. but i can find out a race condition that balancerRunning has been set to false , but the balance thread is still running.
e.g. , `balancer#start` is triggered by cli and `balanceRunning` is atomically set to true, but the balance thread is not been started , at this time , `balancer#stop` is triggered by cli, and `balanceRunning` is atomically set to false.then `balancer#start` continue to execute , and a balance thread will be created and go on. so in this case , `balancerRunning` has been set to false , but the balance thread is still running. please correct me if i am wrong!
but , in any case , start and stop should be synchronized. i will do this in a update commit
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] GlenGeng commented on a change in pull request #2278: HDDS-4926. Support start/stop for container balancer via command line
Posted by GitBox <gi...@apache.org>.
GlenGeng commented on a change in pull request #2278:
URL: https://github.com/apache/ozone/pull/2278#discussion_r638736908
##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/protocol/StorageContainerLocationProtocol.java
##########
@@ -286,6 +286,23 @@ Pipeline createReplicationPipeline(HddsProtos.ReplicationType type,
*/
boolean getReplicationManagerStatus() throws IOException;
+ /**
+ * Start ContainerBalancer.
+ */
+ void startContainerBalancer() throws IOException;
Review comment:
Seems these params are not passed from these RPCs in StorageContainerLocationProtocol ?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi commented on pull request #2278: HDDS-4926. Support start/stop for container balancer via command line
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on pull request #2278:
URL: https://github.com/apache/ozone/pull/2278#issuecomment-866547354
Thanks @JacksonYao287 for working on this. The path LGTM, only several small issues.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] JacksonYao287 commented on pull request #2278: HDDS-4926. Support start/stop for container balancer via command line
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on pull request #2278:
URL: https://github.com/apache/ozone/pull/2278#issuecomment-853678091
thanks @adoroszlai for looking at this!
> Yes, it's unrelated. #2302 fixes it. Please wait for that to be merged first.
will wait for that!
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] siddhantsangwan commented on a change in pull request #2278: HDDS-4926. Support start/stop for container balancer via command line
Posted by GitBox <gi...@apache.org>.
siddhantsangwan commented on a change in pull request #2278:
URL: https://github.com/apache/ozone/pull/2278#discussion_r642267250
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java
##########
@@ -63,16 +68,14 @@ public ContainerBalancer(
/**
* Start ContainerBalancer. Current implementation is incomplete.
- *
- * @param balancerConfiguration Configuration values.
*/
- public void start(ContainerBalancerConfiguration balancerConfiguration) {
+ public void start() {
this.balancerRunning = true;
ozoneConfiguration = new OzoneConfiguration();
// initialise configs
- this.config = balancerConfiguration;
+ //this.config = balancerConfiguration;
Review comment:
Since these parameters are required in multiple classes, we can set these fields in the `ContainerBalancerConfiguration` class and then pass this object to wherever parameters are required. This way, all parameters will be present in one place instead of being get/set in different places. It could be better to maintain all the parameters in a single class. What do you think?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] JacksonYao287 commented on a change in pull request #2278: HDDS-4926. Support start/stop for container balancer via command line
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on a change in pull request #2278:
URL: https://github.com/apache/ozone/pull/2278#discussion_r640532246
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java
##########
@@ -63,16 +68,14 @@ public ContainerBalancer(
/**
* Start ContainerBalancer. Current implementation is incomplete.
- *
- * @param balancerConfiguration Configuration values.
*/
- public void start(ContainerBalancerConfiguration balancerConfiguration) {
+ public void start() {
this.balancerRunning = true;
ozoneConfiguration = new OzoneConfiguration();
// initialise configs
- this.config = balancerConfiguration;
+ //this.config = balancerConfiguration;
Review comment:
@siddhantsangwan Thanks for the review! we can just specify some important parameters for now I think and then keep on adding params as required . we can add more params as we move forward with the implementation.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] lokeshj1703 commented on a change in pull request #2278: HDDS-4926. Support start/stop for container balancer via command line
Posted by GitBox <gi...@apache.org>.
lokeshj1703 commented on a change in pull request #2278:
URL: https://github.com/apache/ozone/pull/2278#discussion_r648186381
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/protocol/StorageContainerLocationProtocolServerSideTranslatorPB.java
##########
@@ -611,6 +638,48 @@ public ReplicationManagerStatusResponseProto getReplicationManagerStatus(
.setIsRunning(impl.getReplicationManagerStatus()).build();
}
+ public StartContainerBalancerResponseProto startContainerBalancer(
+ StartContainerBalancerRequestProto request)
+ throws IOException {
+ double threshold = 0;
Review comment:
NIT: Use 0.0D.
##########
File path: hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/scm/protocolPB/StorageContainerLocationProtocolClientSideTranslatorPB.java
##########
@@ -712,6 +717,67 @@ public boolean getReplicationManagerStatus() throws IOException {
}
+ @Override
+ public boolean startContainerBalancer(double threshold, int idleiterations,
+ int maxDatanodesToBalance, long maxSizeToMove) throws IOException {
+ Preconditions.checkState(threshold >= 0,
Review comment:
NIT. We can also add check threshold < 1.0.
```suggestion
Preconditions.checkState(threshold >= 0.0D && threshold < 1.0D,
```
##########
File path: hadoop-hdds/tools/src/main/java/org/apache/hadoop/hdds/scm/cli/ContainerBalancerCommands.java
##########
@@ -0,0 +1,101 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hdds.scm.cli;
+
+import java.util.concurrent.Callable;
+
+import org.apache.hadoop.hdds.cli.GenericCli;
+import org.apache.hadoop.hdds.cli.HddsVersionProvider;
+import org.apache.hadoop.hdds.cli.OzoneAdmin;
+import org.apache.hadoop.hdds.cli.SubcommandWithParent;
+
+import org.kohsuke.MetaInfServices;
+import picocli.CommandLine.Command;
+import picocli.CommandLine.Model.CommandSpec;
+import picocli.CommandLine.Spec;
+
+/**
+ * Subcommand to group container balancer related operations.
+ *
+ * <p>The balancer is a tool that balances datanode space usage on an Ozone
+ * cluster when some datanodes become full or when new empty nodes join
+ * the cluster. The tool can be run by the cluster administrator
+ * from command line while applications adding and deleting blocks.
+ *
+ * <p>SYNOPSIS
+ * <pre>
+ * To start:
+ * ozone admin containerbalancer start
+ * [ -t/--threshold {@literal <threshold>}]
+ * [ -i/--idleiterations {@literal <idleiterations>}]
+ * Example: ozone admin containerbalancer start
+ * start balancer with a default threshold of 10%
+ * ozone admin containerbalancer start -t 5
+ * start balancer with a threshold of 5%
+ * ozone admin containerbalancer start -i 20
+ * start balancer with maximum 20 consecutive idle iterations
+ * ozone admin containerbalancer start -i -1
+ * run balancer with default threshold infinitely
Review comment:
Since we are not allowing -ve values. We will need to update the documentation.
Even threshold needs to be specified in range [0.0, 1.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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] JacksonYao287 commented on a change in pull request #2278: HDDS-4926. Support start/stop for container balancer via command line
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on a change in pull request #2278:
URL: https://github.com/apache/ozone/pull/2278#discussion_r640553385
##########
File path: hadoop-hdds/tools/src/main/java/org/apache/hadoop/hdds/scm/cli/ContainerBalancerStartSubcommand.java
##########
@@ -0,0 +1,62 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hdds.scm.cli;
+
+import org.apache.hadoop.hdds.cli.HddsVersionProvider;
+import org.apache.hadoop.hdds.scm.client.ScmClient;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import picocli.CommandLine;
+import picocli.CommandLine.Command;
+
+import java.io.IOException;
+
+/**
+ * Handler to start container balancer.
+ */
+@Command(
+ name = "start",
+ description = "Start ContainerBalancer",
+ mixinStandardHelpOptions = true,
+ versionProvider = HddsVersionProvider.class)
+public class ContainerBalancerStartSubcommand extends ScmSubcommand {
+
+ private static final Logger LOG =
+ LoggerFactory.getLogger(ContainerBalancerStartSubcommand.class);
+
+ @CommandLine.Option(names = {"-t", "--threshold"},
+ description = "Threshold target whether the cluster is balanced",
+ defaultValue = "0.1",
Review comment:
updated, PTAL!
there is no any logic code here , just send a command , so maybe no need to add unit test , just like replication manager cli. what is more , acceptance test can be added after we complete container balancer, because by then the acceptance will make more sense! what do you think about？
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org