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

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

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



##########
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:
       I am confused. This is general handle even function. If it is in management mode, then we throw exception. Would this affect the management pipeline handle event?

##########
File path: helix-core/src/main/java/org/apache/helix/controller/dataproviders/ManagementControllerDataProvider.java
##########
@@ -19,9 +19,26 @@
  * under the License.
  */
 
+import java.util.Arrays;
+import java.util.List;
+
+import org.apache.helix.HelixConstants;
+
+/**
+ * Data provider for controller management mode pipeline.
+ */
 public class ManagementControllerDataProvider extends BaseControllerDataProvider {
-  // TODO: implement this class to only refresh required event types
-  public ManagementControllerDataProvider(String clusterName, String name) {
-    super(clusterName, name);
+  private static final List<HelixConstants.ChangeType> FULL_REFRESH_PROPERTIES =

Review comment:
       NIT: This naming could be confusing? With these change types, it does not require full refresh but these properties refresh, right?

##########
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:
       NIT: This monitor will be management pipeline monitor, right? How about regular pipeline monitor update? Shall we update them as well?

##########
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:
       Can we unify these logic into a static util function? Because, more than one places hooked the logic almost same just enable/disable the management mode difference.

##########
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 =
+        event.getAttribute(AttributeName.clusterStatusMonitor.name());
     ManagementControllerDataProvider cache =
         event.getAttribute(AttributeName.ControllerDataProvider.name());
-    if (!HelixUtil.inManagementMode(cache)) {
-      LOG.info("Exiting management mode pipeline for cluster {}", clusterName);
+
+    checkInManagementMode(clusterStatusMonitor, clusterName, cache);
+  }
+
+  private void checkInManagementMode(ClusterStatusMonitor clusterStatusMonitor,
+      String clusterName, ManagementControllerDataProvider cache) throws StageException {
+    // Should exit management mode
+    if (!HelixUtil.inManagementMode(cache.getPauseSignal(), cache.getLiveInstances(),

Review comment:
       As we discussed, pending message of freeze check is necessary to protect whether we can break this pipeline. Otherwise, there would be a race condition can cause lot of unnecessary traffic to ZK.

##########
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:
       Please add some comments here. If we dont break the pipeline, following monitor update needs to be synchronized since the monitor is sharing by multiple pipelines.




-- 
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