jiajunwang commented on a change in pull request #1468:
URL: https://github.com/apache/helix/pull/1468#discussion_r510510178
##########
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
+ handleDeletedTasks(jobResource, jobCfg, jobCtx,
currentInstanceToTaskAssignments, tasksToDrop,
Review comment:
High-levelly, my motivation is to simplify the logic here. The
dispatcher logic looks like a series of if-else blocks. And there are
redundencies and very easy for devs to introduce bugs. I understand it cannot
be done in one day. So what I suggested is just want to prevent us adding
another if-else block to the already long list.
1. it seems that allPartitions is not used by "addGivenUpPartitions" which
is after your new method. So no matter how you modify the allPartitions list,
it will impact the existing use cases anyway.
The only exception is the empty target partitions check. If the user removed
all the tasks, then I think it is OK we mark this job as failure. Or we just
prevent user from deleting all tasks. This is not our target use case.
2. On my understanding, handleDeletedTasks is mainly for 3 tasks.
- Add the deleted tasks to tasksToDrop.
First of all, I do believe we need a generic logic to check for all dropping
tasks in a single place. It is a major issue if we have this kind of logic
everywhere in different methods. One example of the pain is that you actually
need to change the comment of tasksToDrop to fit this new usage.
But that is nice to have for now.
Back to this case, it shall be easy to fill the tasksToDrop list immediately
after we have the filtered allPartitions list, I think. Or you can let the
taskAssignmentCal to do it. Based on it's name, I guess the original design is
to let this class finish such a work.
- Remove the deleted tasks from allPartitions.
As I mentioned, this can be done in the taskAssignmentCal logic.
- Remove the deleted tasks from jobContext.
I haven't fully understand this part. But I don't see there is a way to
remove a task from the jobContext before. So I guess the previous assumption is
that a task can be cancelled but not removed directly. Then is it safe to allow
removing the task now? It seems to trigger race condition. For example, if a
task has been running, and it is requested to be removed at the same time. We
deleted it from the context directly. What will happen? Is it safer just to
cancel it?
I might not understand the workflow here completely. Feel free to set up a
meeting so we can discuss about it.
----------------------------------------------------------------
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]
