jiajunwang commented on a change in pull request #1076:
URL: https://github.com/apache/helix/pull/1076#discussion_r437632067
##########
File path:
helix-core/src/main/java/org/apache/helix/controller/stages/AttributeName.java
##########
@@ -40,5 +40,7 @@
PipelineType,
LastRebalanceFinishTimeStamp,
ControllerDataProvider,
- STATEFUL_REBALANCER
+ STATEFUL_REBALANCER,
+ WORKFLOW_CONFIG_MAP,
Review comment:
Instead of having 2 additional maps that duplicate the fields of
ControllerDataProvider, can we have a specific class presenting the to be GCed
objects?
##########
File path:
helix-core/src/main/java/org/apache/helix/controller/stages/TaskGarbageCollectionStage.java
##########
@@ -23,34 +27,53 @@ public AsyncWorkerType getAsyncWorkerType() {
}
@Override
- public void execute(ClusterEvent event) {
+ public void process(ClusterEvent event) throws Exception {
WorkflowControllerDataProvider dataProvider =
event.getAttribute(AttributeName.ControllerDataProvider.name());
+ event.addAttribute(AttributeName.WORKFLOW_CONFIG_MAP.name(),
+ dataProvider.getWorkflowConfigMap());
Review comment:
Just curious, why are we adding all the workflow/resource contexts here?
IMHO, a better design would be after all workflows, jobs being processed,
add the rest unprocessed workflows, jobs to the async GC thread. Will that be
better?
##########
File path: helix-core/src/main/java/org/apache/helix/task/TaskUtil.java
##########
@@ -1037,6 +1037,53 @@ public static void purgeExpiredJobs(String workflow,
WorkflowConfig workflowConf
setNextJobPurgeTime(workflow, currentTime, purgeInterval,
rebalanceScheduler, manager);
}
+ /**
+ * The function that loops through the all existing workflow contexts and
removes IdealState and
+ * workflow context of the workflow whose workflow config does not exist.
+ * @param workflowConfigMap
+ * @param resourceContextMap
+ * @param manager
+ */
+ public static void workflowGarbageCollection(final Map<String,
WorkflowConfig> workflowConfigMap,
Review comment:
Is this tool related to the TaskGarbageCollectionStage?
It would be better to split the PR if they are not strictly related.
##########
File path:
helix-core/src/main/java/org/apache/helix/controller/stages/TaskGarbageCollectionStage.java
##########
@@ -23,34 +27,53 @@ public AsyncWorkerType getAsyncWorkerType() {
}
@Override
- public void execute(ClusterEvent event) {
+ public void process(ClusterEvent event) throws Exception {
WorkflowControllerDataProvider dataProvider =
event.getAttribute(AttributeName.ControllerDataProvider.name());
+ event.addAttribute(AttributeName.WORKFLOW_CONFIG_MAP.name(),
+ dataProvider.getWorkflowConfigMap());
+ event.addAttribute(AttributeName.RESOURCE_CONTEXT_MAP.name(),
dataProvider.getContexts());
Review comment:
I don't think a reference to the map object itself here is OK. My
understanding is that you want to persist in the cached map before it is
modified in the later stages. In this case, you need to copy the map, not just
pass the reference.
Also, if this change passes our current test, then it may indicate the test
does not cover the race condition case. To justify that this PR really fixes
the race condition, please add the corresponding test case which fails because
of the race condition without the fix.
----------------------------------------------------------------
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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
