[GitHub] [helix] dasahcc commented on a change in pull request #1468: Implement deleteTask API

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

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



##########
File path: helix-core/src/main/java/org/apache/helix/task/JobDispatcher.java
##########
@@ -238,6 +238,10 @@ private ResourceAssignment computeResourceMapping(String jobResource,
 
     updateInstanceToTaskAssignmentsFromContext(jobCtx, currentInstanceToTaskAssignments);
 
+    // Find the tasks that have been removed form the config, add them to TasksToDrop

Review comment:
       "from"

##########
File path: helix-core/src/main/java/org/apache/helix/task/JobDispatcher.java
##########
@@ -478,4 +482,61 @@ private TaskAssignmentCalculator getAssignmentCalculator(JobConfig jobConfig,
     }
     return new FixedTargetTaskAssignmentCalculator(assignableInstanceManager);
   }
+
+  /**
+   * Find the tasks that have been removed from job config, add them to tasksToDrop. If task's
+   * currentState and pending message have been removed, delete the task from job context.
+   * @param jobName
+   * @param jobConfig
+   * @param jobContext
+   * @param currentInstanceToTaskAssignments
+   * @param tasksToDrop
+   * @param currStateOutput
+   * @param allPartitions
+   */
+  private void handleDeletedTasks(String jobName, JobConfig jobConfig, JobContext jobContext,
+      Map<String, SortedSet<Integer>> currentInstanceToTaskAssignments,
+      Map<String, Set<Integer>> tasksToDrop, CurrentStateOutput currStateOutput,
+      Set<Integer> allPartitions) {
+    if (TaskUtil.isGenericTaskJob(jobConfig)) {
+      // Get all partitions existed in the context
+      Set<Integer> contextPartitions = jobContext.getPartitionSet();
+      // Check whether the tasks have been deleted from jobConfig
+      for (Integer partition : contextPartitions) {
+        String partitionID = jobContext.getTaskIdForPartition(partition);
+        if (!jobConfig.getTaskConfigMap().containsKey(partitionID)) {
+          boolean hasCurrentState = false;
+          for (Map.Entry<String, SortedSet<Integer>> instanceToPartitions : currentInstanceToTaskAssignments

Review comment:
       Can we do some pre process work? Make it as task -> list instances. So we dont have to loop all the instances again and again.

##########
File path: helix-core/src/main/java/org/apache/helix/task/JobDispatcher.java
##########
@@ -478,4 +482,61 @@ private TaskAssignmentCalculator getAssignmentCalculator(JobConfig jobConfig,
     }
     return new FixedTargetTaskAssignmentCalculator(assignableInstanceManager);
   }
+
+  /**
+   * Find the tasks that have been removed from job config, add them to tasksToDrop. If task's
+   * currentState and pending message have been removed, delete the task from job context.
+   * @param jobName
+   * @param jobConfig
+   * @param jobContext
+   * @param currentInstanceToTaskAssignments
+   * @param tasksToDrop
+   * @param currStateOutput
+   * @param allPartitions
+   */
+  private void handleDeletedTasks(String jobName, JobConfig jobConfig, JobContext jobContext,
+      Map<String, SortedSet<Integer>> currentInstanceToTaskAssignments,
+      Map<String, Set<Integer>> tasksToDrop, CurrentStateOutput currStateOutput,
+      Set<Integer> allPartitions) {
+    if (TaskUtil.isGenericTaskJob(jobConfig)) {
+      // Get all partitions existed in the context
+      Set<Integer> contextPartitions = jobContext.getPartitionSet();
+      // Check whether the tasks have been deleted from jobConfig
+      for (Integer partition : contextPartitions) {
+        String partitionID = jobContext.getTaskIdForPartition(partition);
+        if (!jobConfig.getTaskConfigMap().containsKey(partitionID)) {

Review comment:
       We dont need to loop everything. We can do 
   
   Set<Integer> partitionRemoved = new HashSet<>(jobContext.getPartitionSet());
   partitionRemoved.removeAll(jobConfig.getTaskConfigMap().keySet());
   
   In this case, we save some checks and some loops.




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