[GitHub] [helix] huizhilu commented on a change in pull request #1793: Move pause and maintenance handling out of controller

[GitBox <gi...@apache.org>]

huizhilu commented on a change in pull request #1793:
URL: https://github.com/apache/helix/pull/1793#discussion_r649775890



##########
File path: helix-core/src/main/java/org/apache/helix/controller/stages/ResourceValidationStage.java
##########
@@ -91,6 +87,29 @@ public void process(ClusterEvent event) throws Exception {
     }
   }
 
+  private void processManagementMode(ClusterEvent event, BaseControllerDataProvider cache)

Review comment:
       No, because the management pipeline does not include this stage. This is data preprocess for default/task. Management does not run it.

##########
File path: helix-core/src/main/java/org/apache/helix/controller/stages/ManagementModeStage.java
##########
@@ -36,13 +38,30 @@
   @Override
   public void process(ClusterEvent event) throws Exception {
     // TODO: implement the stage
+    _eventId = event.getEventId();
     String clusterName = event.getClusterName();
+    ClusterStatusMonitor clusterStatusMonitor =

Review comment:
       This monitor internally is synchronized. We don't have to explicitly synchronized it in the pipeline. The existing monitor update usage in default/task pipelines does not synchronized, either, which is fine.

##########
File path: helix-core/src/main/java/org/apache/helix/controller/stages/ResourceValidationStage.java
##########
@@ -91,6 +87,29 @@ public void process(ClusterEvent event) throws Exception {
     }
   }
 
+  private void processManagementMode(ClusterEvent event, BaseControllerDataProvider cache)
+      throws StageException {
+    // Set cluster status monitor for maintenance mode
+    if (ClusterEventType.ControllerChange.equals(event.getEventType())) {
+      ClusterStatusMonitor monitor = event.getAttribute(AttributeName.clusterStatusMonitor.name());
+      if (monitor != null) {
+        monitor.setMaintenance(cache.isMaintenanceModeEnabled());
+      }
+    }
+
+    // Check if cluster is still in management mode. Eg. there exists any frozen live instance.
+    if (HelixUtil.inManagementMode(cache.getPauseSignal(), cache.getLiveInstances(),

Review comment:
       We cannot combine all, because the logic depends on the result of `inManagement()` and the type of pipeline. But I've put the cluster status monitor logic into the util, which is cleaner now.

##########
File path: helix-core/src/main/java/org/apache/helix/util/HelixUtil.java
##########
@@ -519,13 +519,20 @@ public static InstanceConfig composeInstanceConfig(String instanceName) {
   }
 
   /**
-   * Checks whether or not the cluster is in management mode.
+   * Checks whether or not the cluster is in management mode. It checks pause signal

Review comment:
       Currently we only make it for pause & freeze. In the future when we move M mode to management pipeline, we should also check M mode. A TODO is already added to this API.




-- 
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: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org