You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@helix.apache.org by GitBox <gi...@apache.org> on 2020/12/01 00:17:25 UTC
[GitHub] [helix] alirezazamani opened a new pull request #1568: Mark tasks as DROPPED if assigned participant does not exist
alirezazamani opened a new pull request #1568:
URL: https://github.com/apache/helix/pull/1568
### Issues
- [X] My PR addresses the following Helix issues and references them in the PR description:
Fixes #1567
### Description
- [x] Here are some details about my PR, including screenshots of any UI changes:
In this PR, if the workflow or job is being stopped, but the assigned participant becomes offline for the task, the
task would be marked as DROPPED which allows the workflow and job to go to STOPPED state.
### Tests
- [x] The following tests are written for this issue:
TestStoppingWorkflowAndJob.testStopWorkflowInstanceOffline
- [x] The following is the result of the "mvn test" command on the appropriate module:
Helix-core:
```
[INFO] Tests run: 1252, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 5,400.742 s - in TestSuite
[INFO]
[INFO] Results:
[INFO]
[INFO] Tests run: 1252, Failures: 0, Errors: 0, Skipped: 0
[INFO]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 01:30 h
[INFO] Finished at: 2020-11-30T16:09:06-08:00
[INFO] ------------------------------------------------------------------------
```
The failed test is unrelated to this change and is failing even without this PR.
Helix-rest:
```
[INFO] Tests run: 171, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 263.332 s - in TestSuite
[INFO]
[INFO] Results:
[INFO]
[INFO] Tests run: 171, Failures: 0, Errors: 0, Skipped: 0
[INFO]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 04:27 min
[INFO] Finished at: 2020-11-30T16:14:32-08:00
[INFO] ------------------------------------------------------------------------
```
### Commits
- My commits all reference appropriate Apache Helix GitHub issues in their subject lines. In addition, my commits follow the guidelines from "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)":
1. Subject is separated from body by a blank line
1. Subject is limited to 50 characters (not including Jira issue reference)
1. Subject does not end with a period
1. Subject uses the imperative mood ("add", not "adding")
1. Body wraps at 72 characters
1. Body explains "what" and "why", not "how"
### Code Quality
- My diff has been formatted using helix-style.xml
(helix-style-intellij.xml if IntelliJ IDE is used)
----------------------------------------------------------------
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
[GitHub] [helix] alirezazamani commented on a change in pull request #1568: Mark tasks as DROPPED if assigned participant does not exist
Posted by GitBox <gi...@apache.org>.
alirezazamani commented on a change in pull request #1568:
URL: https://github.com/apache/helix/pull/1568#discussion_r534387668
##########
File path: helix-core/src/main/java/org/apache/helix/task/AbstractTaskDispatcher.java
##########
@@ -837,7 +835,7 @@ private static void addCompletedTasks(Set<Integer> set, JobContext ctx, Iterable
* @param state
* @return
*/
- private boolean isTaskNotInTerminalState(TaskPartitionState state) {
+ protected static boolean isTaskNotInTerminalState(TaskPartitionState state) {
Review comment:
TaskPartitionState is just an enum that lists all the states for the tasks. So I think it is better to keep it here like other similar methods.
----------------------------------------------------------------
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
[GitHub] [helix] alirezazamani commented on a change in pull request #1568: Mark tasks as DROPPED if assigned participant does not exist
Posted by GitBox <gi...@apache.org>.
alirezazamani commented on a change in pull request #1568:
URL: https://github.com/apache/helix/pull/1568#discussion_r533805626
##########
File path: helix-core/src/main/java/org/apache/helix/task/JobDispatcher.java
##########
@@ -317,6 +316,9 @@ private ResourceAssignment computeResourceMapping(String jobResource,
// For delayed tasks, trigger a rebalance event for the closest upcoming ready time
scheduleForNextTask(jobResource, jobCtx, currentTime);
+ // If the assigned instance is no longer live, so mark it as DROPPED in the context
+ markPartitionsWithoutLiveInstance(jobCtx, allPartitions, liveInstances);
Review comment:
It happens when workflow/job is being stopped and while the message is being sent and not processed yet (or the controller has not seen the effect yet) then the live instance is gone. In this case, the job will be stuck in the STOPPING state and the controller does not have a way to mark the task and job to be STOPPED. Note that all new assignment is happening when TargetState is START in handleAdditionTaskAssigment. If not, then assignment logic does not exist and will not be considered. We need to somehow manage this case before that. Note that when the live instance is gone, the controller does not see anything on the instance. I cannot find a better way to achieve this. If you know how it can be resolved, please let me know and I can change it accordingly. Also if you look at handleAdditionalTaskAssignment.filterTask method, it is the same code. I basically changed the place of this code and move it earlier in the pipeline.
----------------------------------------------------------------
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
[GitHub] [helix] jiajunwang commented on a change in pull request #1568: Mark tasks as DROPPED if assigned participant does not exist
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1568:
URL: https://github.com/apache/helix/pull/1568#discussion_r533921946
##########
File path: helix-core/src/main/java/org/apache/helix/task/AbstractTaskDispatcher.java
##########
@@ -935,6 +933,20 @@ protected void markJobFailed(String jobName, JobContext jobContext, WorkflowConf
currentTime);
}
+ protected void markPartitionsWithoutLiveInstance(JobContext jobCtx, Set<Integer> allPartitions,
Review comment:
It seems no one except JobDispatcher needs this method. Can we just move it to there?
##########
File path: helix-core/src/main/java/org/apache/helix/task/AbstractTaskDispatcher.java
##########
@@ -837,7 +835,7 @@ private static void addCompletedTasks(Set<Integer> set, JobContext ctx, Iterable
* @param state
* @return
*/
- private boolean isTaskNotInTerminalState(TaskPartitionState state) {
+ protected static boolean isTaskNotInTerminalState(TaskPartitionState state) {
Review comment:
You might want to move it to TaskPartitionState class.
----------------------------------------------------------------
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
[GitHub] [helix] alirezazamani merged pull request #1568: Mark tasks as DROPPED if assigned participant does not exist
Posted by GitBox <gi...@apache.org>.
alirezazamani merged pull request #1568:
URL: https://github.com/apache/helix/pull/1568
----------------------------------------------------------------
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
[GitHub] [helix] alirezazamani commented on a change in pull request #1568: Mark tasks as DROPPED if assigned participant does not exist
Posted by GitBox <gi...@apache.org>.
alirezazamani commented on a change in pull request #1568:
URL: https://github.com/apache/helix/pull/1568#discussion_r534491531
##########
File path: helix-core/src/main/java/org/apache/helix/task/JobDispatcher.java
##########
@@ -317,6 +316,9 @@ private ResourceAssignment computeResourceMapping(String jobResource,
// For delayed tasks, trigger a rebalance event for the closest upcoming ready time
scheduleForNextTask(jobResource, jobCtx, currentTime);
+ // If the assigned instance is no longer live, so mark it as DROPPED in the context
+ markPartitionsWithoutLiveInstance(jobCtx, allPartitions, liveInstances);
Review comment:
Moved to new place as suggessted.
----------------------------------------------------------------
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
[GitHub] [helix] alirezazamani commented on a change in pull request #1568: Mark tasks as DROPPED if assigned participant does not exist
Posted by GitBox <gi...@apache.org>.
alirezazamani commented on a change in pull request #1568:
URL: https://github.com/apache/helix/pull/1568#discussion_r533805626
##########
File path: helix-core/src/main/java/org/apache/helix/task/JobDispatcher.java
##########
@@ -317,6 +316,9 @@ private ResourceAssignment computeResourceMapping(String jobResource,
// For delayed tasks, trigger a rebalance event for the closest upcoming ready time
scheduleForNextTask(jobResource, jobCtx, currentTime);
+ // If the assigned instance is no longer live, so mark it as DROPPED in the context
+ markPartitionsWithoutLiveInstance(jobCtx, allPartitions, liveInstances);
Review comment:
It happens when workflow/job is being stopped (in STOPPING state) and the message is being sent and not processed yet (or the controller has not seen the effect yet) then the live instance is gone. In this case, the job will be stuck in the STOPPING state and the controller does not have a way to mark the task and job to be STOPPED. Note that all new assignment is happening when TargetState is START in handleAdditionTaskAssigment. If not, then assignment logic does not exist and will not be considered. We need to somehow manage this case before that. Note that when the live instance is gone, the controller does not see anything on the instance. I cannot find a better way to achieve this. If you know how it can be resolved, please let me know and I can change it accordingly. Also if you look at handleAdditionalTaskAssignment.filterTask method, it is the same code. I basically changed the place of this code and move it earlier in the pipeline.
----------------------------------------------------------------
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
[GitHub] [helix] dasahcc commented on a change in pull request #1568: Mark tasks as DROPPED if assigned participant does not exist
Posted by GitBox <gi...@apache.org>.
dasahcc commented on a change in pull request #1568:
URL: https://github.com/apache/helix/pull/1568#discussion_r533652992
##########
File path: helix-core/src/main/java/org/apache/helix/task/JobDispatcher.java
##########
@@ -317,6 +316,9 @@ private ResourceAssignment computeResourceMapping(String jobResource,
// For delayed tasks, trigger a rebalance event for the closest upcoming ready time
scheduleForNextTask(jobResource, jobCtx, currentTime);
+ // If the assigned instance is no longer live, so mark it as DROPPED in the context
+ markPartitionsWithoutLiveInstance(jobCtx, allPartitions, liveInstances);
Review comment:
This looks like an override of processing result... Can we have the logic in somewhere in scheduling task? This is more like a patching after scheduling computation. It waste the scheduling computation if instance is not live.
----------------------------------------------------------------
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
[GitHub] [helix] alirezazamani commented on a change in pull request #1568: Mark tasks as DROPPED if assigned participant does not exist
Posted by GitBox <gi...@apache.org>.
alirezazamani commented on a change in pull request #1568:
URL: https://github.com/apache/helix/pull/1568#discussion_r533668256
##########
File path: helix-core/src/main/java/org/apache/helix/task/JobDispatcher.java
##########
@@ -317,6 +316,9 @@ private ResourceAssignment computeResourceMapping(String jobResource,
// For delayed tasks, trigger a rebalance event for the closest upcoming ready time
scheduleForNextTask(jobResource, jobCtx, currentTime);
+ // If the assigned instance is no longer live, so mark it as DROPPED in the context
+ markPartitionsWithoutLiveInstance(jobCtx, allPartitions, liveInstances);
Review comment:
We have this logic in the handleAdditionalTasks in filterTask method. Since that part of the code only hits when TargetState is equal to start, I moved the logic here so in all conditions we evaluate it. If the instance is not live, we still need to schedule the task if the task needs to be scheduled on other instances. If the instance is not live, then we do not see the task in current states which means we would no process the task in this case.
----------------------------------------------------------------
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
[GitHub] [helix] alirezazamani commented on a change in pull request #1568: Mark tasks as DROPPED if assigned participant does not exist
Posted by GitBox <gi...@apache.org>.
alirezazamani commented on a change in pull request #1568:
URL: https://github.com/apache/helix/pull/1568#discussion_r534387905
##########
File path: helix-core/src/main/java/org/apache/helix/task/AbstractTaskDispatcher.java
##########
@@ -935,6 +933,20 @@ protected void markJobFailed(String jobName, JobContext jobContext, WorkflowConf
currentTime);
}
+ protected void markPartitionsWithoutLiveInstance(JobContext jobCtx, Set<Integer> allPartitions,
Review comment:
Sure. Moved to jobDispatcher.
----------------------------------------------------------------
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
[GitHub] [helix] dasahcc commented on a change in pull request #1568: Mark tasks as DROPPED if assigned participant does not exist
Posted by GitBox <gi...@apache.org>.
dasahcc commented on a change in pull request #1568:
URL: https://github.com/apache/helix/pull/1568#discussion_r534411565
##########
File path: helix-core/src/main/java/org/apache/helix/task/JobDispatcher.java
##########
@@ -317,6 +316,9 @@ private ResourceAssignment computeResourceMapping(String jobResource,
// For delayed tasks, trigger a rebalance event for the closest upcoming ready time
scheduleForNextTask(jobResource, jobCtx, currentTime);
+ // If the assigned instance is no longer live, so mark it as DROPPED in the context
+ markPartitionsWithoutLiveInstance(jobCtx, allPartitions, liveInstances);
Review comment:
If this is the only scenario, my suggestion is moving the logic to following check in JobDispatcher. It checks whether we can determine a job is already stopped or not. It is also the right time that we check whether the instance assigned with tasks are gone or not. If it is gone, we can mark it as DROPPED.
public static boolean checkJobStopped(JobContext jobContext) {
for (int partition : jobContext.getPartitionSet()) {
TaskPartitionState taskState = jobContext.getPartitionState(partition);
if (taskState == TaskPartitionState.RUNNING) {
return false;
}
}
return true;
}
----------------------------------------------------------------
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
[GitHub] [helix] dasahcc commented on a change in pull request #1568: Mark tasks as DROPPED if assigned participant does not exist
Posted by GitBox <gi...@apache.org>.
dasahcc commented on a change in pull request #1568:
URL: https://github.com/apache/helix/pull/1568#discussion_r533788093
##########
File path: helix-core/src/main/java/org/apache/helix/task/JobDispatcher.java
##########
@@ -317,6 +316,9 @@ private ResourceAssignment computeResourceMapping(String jobResource,
// For delayed tasks, trigger a rebalance event for the closest upcoming ready time
scheduleForNextTask(jobResource, jobCtx, currentTime);
+ // If the assigned instance is no longer live, so mark it as DROPPED in the context
+ markPartitionsWithoutLiveInstance(jobCtx, allPartitions, liveInstances);
Review comment:
I understand the complexity of doing the check. So my question is when could this happening? We need to think about to put to the right place. Add here works, but it looks like a "saving action" without understand any assignment logic. I would prefer to understand the scenario and move the check logic for assignment there.
----------------------------------------------------------------
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
[GitHub] [helix] alirezazamani commented on pull request #1568: Mark tasks as DROPPED if assigned participant does not exist
Posted by GitBox <gi...@apache.org>.
alirezazamani commented on pull request #1568:
URL: https://github.com/apache/helix/pull/1568#issuecomment-737573287
This PR is ready to be merged.
Final commit message:
Mark tasks as DROPPED if assigned participant is not live
In this commit, when the workflows or jobs are being stopped,
if the assigned participant becomes offline for the task, the
task would be marked as DROPPED which allows the workflows
and jobs to go to STOPPED 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