[GitHub] [helix] zhangmeng916 commented on a change in pull request #1769: Add management mode pipeline registry and switch logic

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

zhangmeng916 commented on a change in pull request #1769:
URL: https://github.com/apache/helix/pull/1769#discussion_r646025564



##########
File path: helix-core/src/main/java/org/apache/helix/controller/stages/ResourceValidationStage.java
##########
@@ -41,6 +42,15 @@ public void process(ClusterEvent event) throws Exception {
     if (cache == null) {
       throw new StageException("Missing attributes in event:" + event + ". Requires DataCache");
     }
+
+    // Check if cluster is still in management mode. Eg. there exists any frozen live instance.
+    if (!cache.shouldRunManagementPipeline()) {
+      // Trigger an immediate management mode pipeline.
+      RebalanceUtil.setRunManagementModePipeline(event.getClusterName(), true);
+      RebalanceUtil.scheduleOnDemandPipeline(event.getClusterName(), 0L);
+      throw new StageException("Pipeline should not be run because cluster is in management mode");

Review comment:
       I don't quite get the logic here. Where we always throw an exception?

##########
File path: helix-core/src/main/java/org/apache/helix/util/RebalanceUtil.java
##########
@@ -145,6 +145,26 @@
     return result;
   }
 
+  /**
+   * Whether or not witch controller to run management mode pipeline.
+   *
+   * @param clusterName target cluster name
+   * @param turnOn turn on/off management mode pipeline
+   */
+  public static void setRunManagementModePipeline(String clusterName, boolean turnOn) {
+    GenericHelixController leaderController =
+        GenericHelixController.getLeaderController(clusterName);
+    if (leaderController != null) {
+      LOG.info("Switching management mode pipeline for cluster={}, turnOn={}", clusterName,
+          turnOn);
+      leaderController.setRunManagementModePipeline(turnOn);
+    } else {
+      LOG.error(
+          "Failed to switch management mode pipeline. Controller for cluster {} does not exist",
+          clusterName);

Review comment:
       Can you double check the java doc and comment and make it more precise? I think you mean switch to management mode.

##########
File path: helix-core/src/main/java/org/apache/helix/controller/GenericHelixController.java
##########
@@ -282,18 +290,28 @@ public static void removeController(GenericHelixController controller) {
    */
   public GenericHelixController() {
     this(createDefaultRegistry(Pipeline.Type.DEFAULT.name()),
-        createTaskRegistry(Pipeline.Type.TASK.name()));
+        createTaskRegistry(Pipeline.Type.TASK.name()),
+        createManagementModeRegistry(Pipeline.Type.MANAGEMENT_MODE.name()));
   }
 
   public GenericHelixController(String clusterName) {
     this(createDefaultRegistry(Pipeline.Type.DEFAULT.name()),
-        createTaskRegistry(Pipeline.Type.TASK.name()), clusterName,
+        createTaskRegistry(Pipeline.Type.TASK.name()),
+        createManagementModeRegistry(Pipeline.Type.MANAGEMENT_MODE.name()),
+        clusterName,
         Sets.newHashSet(Pipeline.Type.TASK, Pipeline.Type.DEFAULT));
   }
 
-  public GenericHelixController(String clusterName, Set<Pipeline.Type> enabledPipelins) {
+  public GenericHelixController(String clusterName, Set<Pipeline.Type> enabledPipelines) {
     this(createDefaultRegistry(Pipeline.Type.DEFAULT.name()),
-        createTaskRegistry(Pipeline.Type.TASK.name()), clusterName, enabledPipelins);
+        createTaskRegistry(Pipeline.Type.TASK.name()),
+        createManagementModeRegistry(Pipeline.Type.MANAGEMENT_MODE.name()),
+        clusterName,
+        enabledPipelines);
+  }
+
+  public void setRunManagementModePipeline(boolean turnOn) {
+    _runManagementModePipeline = turnOn;

Review comment:
       Can we change this to `enabled` or similar to follow the convention?

##########
File path: helix-core/src/main/java/org/apache/helix/controller/GenericHelixController.java
##########
@@ -775,12 +834,34 @@ private void handleEvent(ClusterEvent event, BaseControllerDataProvider dataProv
 
     _helixManager = manager;
 
-    // TODO If init controller with paused = true, it may not take effect immediately
-    // _paused is default false. If any events come before controllerChangeEvent, the controller
-    // will be excuting in un-paused mode. Which might not be the config in ZK.
-    if (_paused) {
-      logger.info("Cluster " + manager.getClusterName() + " is paused. Ignoring the event:" + event
-          .getEventType());
+    // Prepare ClusterEvent
+    // TODO (harry): this is a temporal workaround - after controller is separated we should not
+    // have this instanceof clauses
+    List<Pipeline> pipelines;
+    boolean isTaskFrameworkPipeline = false;
+    Pipeline.Type pipelineType;
+
+    if (dataProvider instanceof ResourceControllerDataProvider) {
+      pipelines = _registry.getPipelinesForEvent(event.getEventType());
+      pipelineType = Pipeline.Type.DEFAULT;
+    } else if (dataProvider instanceof WorkflowControllerDataProvider) {
+      pipelines = _taskRegistry.getPipelinesForEvent(event.getEventType());
+      isTaskFrameworkPipeline = true;
+      pipelineType = Pipeline.Type.TASK;
+    } else if (dataProvider instanceof ManagementControllerDataProvider) {
+      pipelines = _managementModeRegistry.getPipelinesForEvent(event.getEventType());
+      pipelineType = Pipeline.Type.MANAGEMENT_MODE;
+    } else {
+      logger.warn(String
+          .format("No %s pipeline to run for event: %s::%s", dataProvider.getPipelineName(),
+              event.getEventType(), event.getEventId()));
+      return;
+    }
+
+    // Default/task pipelines should not run while management mode pipeline is enabled to run
+    if (_runManagementModePipeline && !Pipeline.Type.MANAGEMENT_MODE.equals(pipelineType)) {

Review comment:
       Do we need to check the opposite? I mean how to ensure management mode pipeline doesn't run during the other two pipelines?

##########
File path: helix-core/src/main/java/org/apache/helix/controller/stages/ManagementModeStage.java
##########
@@ -0,0 +1,44 @@
+package org.apache.helix.controller.stages;
+
+/*
+ * 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
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * 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.
+ */
+
+import org.apache.helix.controller.dataproviders.ManagementControllerDataProvider;
+import org.apache.helix.controller.pipeline.AbstractBaseStage;
+import org.apache.helix.util.RebalanceUtil;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class ManagementModeStage extends AbstractBaseStage {

Review comment:
       Please remember to add a test case for this stage 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: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org