[GitHub] [helix] dasahcc commented on a change in pull request #1600: Eliminate redundant workflow context writes

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

dasahcc commented on a change in pull request #1600:
URL: https://github.com/apache/helix/pull/1600#discussion_r553521658



##########
File path: helix-core/src/main/java/org/apache/helix/task/WorkflowContext.java
##########
@@ -200,23 +222,37 @@ public long getFinishTime() {
   }
 
   public void setLastScheduledSingleWorkflow(String workflow) {
-    _record.setSimpleField(WorkflowContextProperties.LAST_SCHEDULED_WORKFLOW.name(), workflow);
+    String lastScheduleWorkflow =
+        _record.getSimpleField(WorkflowContextProperties.LAST_SCHEDULED_WORKFLOW.name());
+    if (!workflow.equals(lastScheduleWorkflow)) {
+      _record.setSimpleField(WorkflowContextProperties.LAST_SCHEDULED_WORKFLOW.name(), workflow);
+      markWorkflowContextAsModified();
+    }
     // Record scheduled workflow into the history list as well
-    List<String> workflows = getScheduledWorkflows();
-    if (workflows == null) {
-      workflows = new ArrayList<String>();
-      _record.setListField(WorkflowContextProperties.SCHEDULED_WORKFLOWS.name(), workflows);
+    List<String> scheduledWorkflows = getScheduledWorkflows();
+    if (scheduledWorkflows == null) {
+      scheduledWorkflows = new ArrayList<String>();
+      _record.setListField(WorkflowContextProperties.SCHEDULED_WORKFLOWS.name(),
+          scheduledWorkflows);
+      markWorkflowContextAsModified();
+    }
+    if (!scheduledWorkflows.contains(workflow)) {

Review comment:
       Same here. Either you add the workflows in previous block. Or remove       markWorkflowContextAsModified() in previous one.

##########
File path: helix-core/src/main/java/org/apache/helix/task/WorkflowContext.java
##########
@@ -81,15 +85,20 @@ public void setJobState(String job, TaskState s) {
     if (states == null) {
       states = new TreeMap<String, String>();
       _record.setMapField(WorkflowContextProperties.JOB_STATES.name(), states);
+      markWorkflowContextAsModified();
+    }
+    if (!s.name().equals(states.get(job))) {

Review comment:
       states.get(job) could be null, right? If the previous block happens.

##########
File path: helix-core/src/main/java/org/apache/helix/task/WorkflowContext.java
##########
@@ -81,15 +85,20 @@ public void setJobState(String job, TaskState s) {
     if (states == null) {
       states = new TreeMap<String, String>();
       _record.setMapField(WorkflowContextProperties.JOB_STATES.name(), states);
+      markWorkflowContextAsModified();

Review comment:
       This is not necessary. If the block has been entered, it definitely will go through next block.

##########
File path: helix-core/src/main/java/org/apache/helix/task/WorkflowContext.java
##########
@@ -48,22 +48,26 @@
     LAST_PURGE_TIME,
     StartTime, // TODO this should be named JOB_SCHEDULED_START_TIME, it's not the actual start time of the job
     NAME
-    }
+  }
 
   public static final int NOT_STARTED = -1;
   public static final int UNFINISHED = -1;
 
+  // Note: This field needs to be set if any of the workflow context fields have been changed.
+  // Otherwise, the context will not be written to ZK by the controller.
+  private boolean isModified;
+
   public WorkflowContext(ZNRecord record) {
     super(record);
+    isModified = false;
   }
 
   public void setWorkflowState(TaskState s) {
     String workflowState = _record.getSimpleField(WorkflowContextProperties.STATE.name());
-    if (workflowState == null) {
-      _record.setSimpleField(WorkflowContextProperties.STATE.name(), s.name());
-    } else if (!workflowState.equals(TaskState.FAILED.name()) && !workflowState
-        .equals(TaskState.COMPLETED.name())) {
+    if (workflowState == null || (!workflowState.equals(TaskState.FAILED.name())

Review comment:
       We can move the definition of terminal states from Workflow/Job Dispatcher to some constant file. Then let's check whether the state whether in the terminal state.




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