siddhantsangwan commented on code in PR #3423:
URL: https://github.com/apache/ozone/pull/3423#discussion_r878017833
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java:
##########
@@ -801,24 +852,70 @@ private void resetState() {
/**
* 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 enters safe mode.
+ * follower or enters safe mode. Starts ContainerBalancer if the current
+ * SCM becomes leader, is out of safe mode and balancer should run.
*/
@Override
public void notifyStatusChanged() {
- if (!scmContext.isLeader() || scmContext.isInSafeMode()) {
- if (isBalancerRunning()) {
- stopBalancingThread();
+ lock.lock();
+ try {
+ if (!scmContext.isLeader() || scmContext.isInSafeMode()) {
+ if (isBalancerRunning()) {
+ stop();
+ }
+ } else {
+ if (shouldRun()) {
+ try {
+ start();
+ } catch (IllegalContainerBalancerStateException |
+ InvalidContainerBalancerConfigurationException e) {
+ LOG.warn("Could not start ContainerBalancer on raft/safe-mode " +
+ "status change.", e);
+ }
+ }
}
+ } finally {
+ lock.unlock();
}
}
/**
- * Checks if ContainerBalancer should run.
- * @return false
+ * Checks if ContainerBalancer should start (after a leader change, restart
+ * etc.) by reading persisted state.
+ * @return true if the persisted state is true, otherwise false
*/
@Override
public boolean shouldRun() {
- return false;
+ try {
+ ContainerBalancerConfigurationProto proto =
+ readConfiguration(ContainerBalancerConfigurationProto.class);
+ if (proto == null) {
+ LOG.warn("Could not find persisted configuration for {} when checking"
+
+ " if ContainerBalancer should run. ContainerBalancer should not " +
+ "run now.", this.getServiceName());
+ return false;
+ }
+ return proto.getShouldRun();
+ } catch (IOException e) {
+ LOG.warn("Could not read persisted configuration for checking if " +
+ "ContainerBalancer should start. ContainerBalancer should not start"
+
+ " now.", e);
+ return false;
+ }
Review Comment:
Agreed. However, since `start` might be called independently of `shouldRun`
(like in SCMServiceManager), our only option is calling `shouldRun` in `start`
itself. So, if we want to validate only once, we'll have to do this in
`shouldRun`. But this has the following problems:
1. Storing configuration inside `shouldRun` can be unexpected for users of
this method.
2. We need access to the configuration proto in `start` for
ContainerBalancerConfigurationProto#getNextIterationIndex.
--
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]
