[GitHub] [helix] jiajunwang commented on a change in pull request #1520: Implement job context garbage collection

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

jiajunwang commented on a change in pull request #1520:
URL: https://github.com/apache/helix/pull/1520#discussion_r522356659



##########
File path: helix-core/src/main/java/org/apache/helix/task/TaskUtil.java
##########
@@ -1142,6 +1142,35 @@ public static void workflowGarbageCollection(final Set<String> toBePurgedWorkflo
     }
   }
 
+  /**
+   * The function that removes IdealStates and job contexts of the jobs that need to be
+   * deleted.
+   * @param toBePurgedJobs
+   * @param manager
+   */
+  public static void jobGarbageCollection(final Set<String> toBePurgedJobs,

Review comment:
       I feel purgeExpiredJobs has a more complete logic to process if the removal fails. Why not using it directly. In this case, you won't need 2 lists in the event object. You can merge the job with no config to the expired job for purging.
   Or is there any conflict logic there?

##########
File path: helix-core/src/main/java/org/apache/helix/task/TaskUtil.java
##########
@@ -1142,6 +1142,35 @@ public static void workflowGarbageCollection(final Set<String> toBePurgedWorkflo
     }
   }
 
+  /**
+   * The function that removes IdealStates and job contexts of the jobs that need to be
+   * deleted.
+   * @param toBePurgedJobs
+   * @param manager
+   */
+  public static void jobGarbageCollection(final Set<String> toBePurgedJobs,
+      final HelixManager manager) {
+    HelixDataAccessor accessor = manager.getHelixDataAccessor();
+    HelixPropertyStore<ZNRecord> propertyStore = manager.getHelixPropertyStore();
+
+    for (String jobName : toBePurgedJobs) {
+      LOG.warn(
+          "JobContext exists for job {}. However, job Config is missing! Deleting the JobContext and IdealState!!",
+          jobName);
+
+      // TODO: We dont need this in the future when TF is not relying on IS/EV anymore.
+      if (!cleanupJobIdealStateExtView(accessor, jobName)) {
+        LOG.warn("Error occurred while trying to remove job idealstate/externalview for {}.",
+            jobName);
+        continue;
+      }
+
+      if (!removeJobContext(propertyStore, jobName)) {
+        LOG.warn("Error occurred while trying to remove job context for {}.", jobName);

Review comment:
       This section is almost identical as removeJob(), could you please merge?

##########
File path: helix-core/src/main/java/org/apache/helix/controller/stages/AttributeName.java
##########
@@ -48,5 +48,7 @@
   // This attribute should only be used in TaskGarbageCollectionStage, misuse could cause race conditions.
   TO_BE_PURGED_WORKFLOWS,
   // This attribute should only be used in TaskGarbageCollectionStage, misuse could cause race conditions.
+  JOBS_WITHOUT_CONFIG,

Review comment:
       One minor question, why not call it TO_BE_PURGED_JOB directly?




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