junkaixue commented on a change in pull request #1769:
URL: https://github.com/apache/helix/pull/1769#discussion_r643696871
##########
File path: helix-core/src/main/java/org/apache/helix/controller/GenericHelixController.java
##########
@@ -628,18 +652,44 @@ private static PipelineRegistry createTaskRegistry(String pipelineName) {
}
}
+ private static PipelineRegistry createManagementModeRegistry(String pipelineName) {
+ logger.info("Creating management mode registry");
+ synchronized (GenericHelixController.class) {
+ // cluster data cache refresh
+ Pipeline dataRefresh = new Pipeline(pipelineName);
+ dataRefresh.addStage(new ReadClusterDataStage());
+
+ // cluster management mode process
+ Pipeline managementMode = new Pipeline(pipelineName);
+ managementMode.addStage(new ManagementModeStage());
+
+ PipelineRegistry registry = new PipelineRegistry();
+ Arrays.asList(
Review comment:
where is onController change?
##########
File path: helix-core/src/main/java/org/apache/helix/controller/GenericHelixController.java
##########
@@ -688,6 +738,21 @@ private GenericHelixController(PipelineRegistry registry, PipelineRegistry taskR
_taskEventThread = null;
}
+ if (_enabledPipelineTypes.contains(Pipeline.Type.MANAGEMENT_MODE)) {
Review comment:
The code her is exact same logic with previous two, just variable different. Can we have a private function to abstract the common logic and make the code more clear?
##########
File path: helix-core/src/main/java/org/apache/helix/controller/GenericHelixController.java
##########
@@ -775,12 +840,35 @@ 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 (!Pipeline.Type.MANAGEMENT_MODE.equals(pipelineType)
+ && _managementControllerDataProvider.shouldRunManagementPipeline()) {
Review comment:
This is not a good idea to take the cache for management pipelines. It can cause race conditions.
I would suggest to use the cache from the event since this function is general function for all three types of caches.
##########
File path: helix-core/src/main/java/org/apache/helix/controller/GenericHelixController.java
##########
@@ -1227,6 +1299,19 @@ private void pushToEventQueues(ClusterEventType eventType, NotificationContext c
for (Map.Entry<String, Object> attr : eventAttributes.entrySet()) {
event.addAttribute(attr.getKey(), attr.getValue());
}
+
+ // Management mode event will force management mode pipeline.
+ if (ClusterEventType.ManagementModePipeline.equals(eventType)
+ || _managementControllerDataProvider.shouldRunManagementPipeline()) {
Review comment:
Same here, better not use the object reference. We have the cache dedicated in event. Let's bound the operation in the function scope.
--
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