lokeshj1703 commented on a change in pull request #3153:
URL: https://github.com/apache/ozone/pull/3153#discussion_r818558543
##########
File path:
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java
##########
@@ -740,14 +708,126 @@ private void incSizeSelectedForMoving(DatanodeDetails
source,
findTargetStrategy.increaseSizeEntering(target, size);
}
+ /**
+ * Receives a notification for raft or safe mode related status changes.
+ * Stops ContainerBalancer if it's running and the current SCM becomes a
+ * follower or is in safe mode.
+ */
+ @Override
+ public void notifyStatusChanged() {
+ if (!checkLeaderAndSafeMode()) {
Review comment:
Should we just call stopBalancer here?
##########
File path:
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java
##########
@@ -740,14 +708,126 @@ private void incSizeSelectedForMoving(DatanodeDetails
source,
findTargetStrategy.increaseSizeEntering(target, size);
}
+ /**
+ * Receives a notification for raft or safe mode related status changes.
+ * Stops ContainerBalancer if it's running and the current SCM becomes a
+ * follower or is in safe mode.
+ */
+ @Override
+ public void notifyStatusChanged() {
+ if (!checkLeaderAndSafeMode()) {
+ if (isBalancerRunning()) {
+ stopBalancer();
+ }
+ }
+ }
+
+ /**
+ * No use of this method, currently.
+ * @return true
+ */
+ @Override
+ public boolean shouldRun() {
+ return true;
+ }
+
+ /**
+ * @return Name of this service.
+ */
+ @Override
+ public String getServiceName() {
+ return ContainerBalancer.class.getSimpleName();
+ }
+
+ /**
+ * Starts Container Balancer after checking its state and validating
+ * configurations.
+ * @throws RuntimeException if ContainerBalancer is not in a
start-appropriate
+ * state or {@link ContainerBalancerConfiguration} config file is
+ * incorrectly configured
+ */
+ @Override
+ public void start() throws RuntimeException {
Review comment:
We do not need to declare RuntimeException.
##########
File path:
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java
##########
@@ -740,14 +708,126 @@ private void incSizeSelectedForMoving(DatanodeDetails
source,
findTargetStrategy.increaseSizeEntering(target, size);
}
+ /**
+ * Receives a notification for raft or safe mode related status changes.
+ * Stops ContainerBalancer if it's running and the current SCM becomes a
+ * follower or is in safe mode.
+ */
+ @Override
+ public void notifyStatusChanged() {
+ if (!checkLeaderAndSafeMode()) {
+ if (isBalancerRunning()) {
+ stopBalancer();
+ }
+ }
+ }
+
+ /**
+ * No use of this method, currently.
+ * @return true
+ */
+ @Override
+ public boolean shouldRun() {
+ return true;
+ }
+
+ /**
+ * @return Name of this service.
+ */
+ @Override
+ public String getServiceName() {
+ return ContainerBalancer.class.getSimpleName();
+ }
+
+ /**
+ * Starts Container Balancer after checking its state and validating
+ * configurations.
+ * @throws RuntimeException if ContainerBalancer is not in a
start-appropriate
+ * state or {@link ContainerBalancerConfiguration} config file is
+ * incorrectly configured
+ */
+ @Override
+ public void start() throws RuntimeException {
Review comment:
Also I am not sure if this should be RuntimeException since this may not
be caught in the stack trace.
##########
File path:
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java
##########
@@ -740,14 +708,126 @@ private void incSizeSelectedForMoving(DatanodeDetails
source,
findTargetStrategy.increaseSizeEntering(target, size);
}
+ /**
+ * Receives a notification for raft or safe mode related status changes.
+ * Stops ContainerBalancer if it's running and the current SCM becomes a
+ * follower or is in safe mode.
+ */
+ @Override
+ public void notifyStatusChanged() {
+ if (!checkLeaderAndSafeMode()) {
+ if (isBalancerRunning()) {
+ stopBalancer();
+ }
+ }
+ }
+
+ /**
+ * No use of this method, currently.
+ * @return true
+ */
+ @Override
+ public boolean shouldRun() {
+ return true;
+ }
+
+ /**
+ * @return Name of this service.
+ */
+ @Override
+ public String getServiceName() {
+ return ContainerBalancer.class.getSimpleName();
+ }
+
+ /**
+ * Starts Container Balancer after checking its state and validating
+ * configurations.
+ * @throws RuntimeException if ContainerBalancer is not in a
start-appropriate
+ * state or {@link ContainerBalancerConfiguration} config file is
+ * incorrectly configured
+ */
+ @Override
+ public void start() throws RuntimeException {
+ lock.lock();
+ try {
+ if (!canRun()) {
+ LOG.warn("Cannot start ContainerBalancer in the current state.");
+ throw new RuntimeException("Cannot start ContainerBalancer");
Review comment:
If a balancer is already running then we can avoid throwing an exception
and maybe just log 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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
