You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "georgew5656 (via GitHub)" <gi...@apache.org> on 2023/08/22 17:39:16 UTC
[PR] Move some lifecycle management from doTask -> shutdown for the mm-less task runner (druid)
georgew5656 opened a new pull request, #14895:
URL: https://github.com/apache/druid/pull/14895
### Description
The mm-less task runner differs in behavior from the other runners that run on the overlord because it tries to handle the cleanup lifecycle on its own by immediately deleting k8s jobs and clearing it's tasks map as soon as it has finished running a task.
The HttpRemoteTaskRunner and RemoteTaskRunner don't do this. Instead, they rely on the TaskQueue's handlers for the futures that all the task runners return to call shutdown on the task once it has completed.
Updating the mm-less task runner to use logic more similar to the other task runners has a couple benefits.
- The task location (including the k8sPodName) is successfully persisted to taskStorage in TaskQueue.notifyStatus. Currently the mm-less task runner reports no location in this function call because its run lifecycle will have already cleaned up the K8s Job and its tasks map.
- Currently, when a task completes, the taskQueue handler will try to call shutdown on the k8s task runner after the runner has already shut down the task. this creates a bunch of "Ignoring request to cancel unknown task" logs and in general seems likely to cause unexpected behavior in the future. Changing the logic will remove this duplication.
**Changes**
- In KubernetesPeonLifecycle, stop calling shutdown (to delete the K8s job) after a job has finished.
- In KubernetesTaskRunner.doTask, stop removing the taskId from tasks in the logic of the run future.
- In KubernetesTaskRunner.shutdown, remove taskId from tasks in addition to calling shutdown on the job. When taskQueue calls this shutdown function, everything in the task will be cleaned up as expected.
- Remove the shutdownRequested flag from KubernetesWorkItem since we can now treat the presence (or lack of presence) of the taskId in the tasks map as a indicator of whether the task was shutdown.
#### Release note
Update mm-less task runner lifecycle logic to better match the logic in the HTTP and Zookeeper worker task runners.
##### Key changed/added classes in this PR
* `KubernetesPeonLifecycle`
* `KubernetesTaskRunner`
* `KubernetesWorkItem`
<hr>
<!-- Check the items by putting "x" in the brackets for the done things. Not all of these items apply to every PR. Remove the items which are not done or not relevant to the PR. None of the items from the checklist below are strictly necessary, but it would be very helpful if you at least self-review the PR. -->
This PR has:
- [X] been self-reviewed.
- [ ] using the [concurrency checklist](https://github.com/apache/druid/blob/master/dev/code-review/concurrency.md) (Remove this item if the PR doesn't have any relation to concurrency.)
- [ ] added documentation for new or modified features or behaviors.
- [ ] a release note entry in the PR description.
- [ ] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
- [ ] added or updated version, license, or notice information in [licenses.yaml](https://github.com/apache/druid/blob/master/dev/license.md)
- [ ] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [X] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for [code coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
- [ ] added integration tests.
- [X] been tested in a test Druid cluster.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Move some lifecycle management from doTask -> shutdown for the mm-less task runner (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #14895:
URL: https://github.com/apache/druid/pull/14895#discussion_r1303782709
##########
extensions-contrib/kubernetes-overlord-extensions/src/main/java/org/apache/druid/k8s/overlord/KubernetesPeonLifecycle.java:
##########
@@ -188,7 +186,7 @@ protected synchronized TaskStatus join(long timeout) throws IllegalStateExceptio
*/
protected void shutdown()
{
- if (State.PENDING.equals(state.get()) || State.RUNNING.equals(state.get())) {
+ if (State.PENDING.equals(state.get()) || State.RUNNING.equals(state.get()) || State.STOPPED.equals(state.get())) {
Review Comment:
Why do we need to handle STOPPED state here? Wouldn't the job have already finished?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Move some lifecycle management from doTask -> shutdown for the mm-less task runner (druid)
Posted by "georgew5656 (via GitHub)" <gi...@apache.org>.
georgew5656 commented on code in PR #14895:
URL: https://github.com/apache/druid/pull/14895#discussion_r1304671894
##########
extensions-contrib/kubernetes-overlord-extensions/src/main/java/org/apache/druid/k8s/overlord/KubernetesPeonLifecycle.java:
##########
@@ -188,7 +186,7 @@ protected synchronized TaskStatus join(long timeout) throws IllegalStateExceptio
*/
protected void shutdown()
{
- if (State.PENDING.equals(state.get()) || State.RUNNING.equals(state.get())) {
+ if (State.PENDING.equals(state.get()) || State.RUNNING.equals(state.get()) || State.STOPPED.equals(state.get())) {
Review Comment:
this is saying thats its okay to shutdown if its stopped
--
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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Move some lifecycle management from doTask -> shutdown for the mm-less task runner (druid)
Posted by "YongGang (via GitHub)" <gi...@apache.org>.
YongGang commented on code in PR #14895:
URL: https://github.com/apache/druid/pull/14895#discussion_r1305875753
##########
extensions-contrib/kubernetes-overlord-extensions/src/main/java/org/apache/druid/k8s/overlord/KubernetesTaskRunner.java:
##########
@@ -271,6 +262,10 @@ public void shutdown(String taskid, String reason)
return;
}
+ synchronized (tasks) {
Review Comment:
nit: the code can be simplified:
```
KubernetesWorkItem workItem;
synchronized (tasks) {
workItem = tasks.remove(taskid);
}
if (workItem == null) {
log.info("Ignoring request to cancel unknown task [%s]", taskid);
return;
}
```
--
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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Move some lifecycle management from doTask -> shutdown for the mm-less task runner (druid)
Posted by "georgew5656 (via GitHub)" <gi...@apache.org>.
georgew5656 commented on PR #14895:
URL: https://github.com/apache/druid/pull/14895#issuecomment-1692158365
> Looks good. Left a minor comment.
>
> @georgew5656 , what would be the side effect of this if the `TaskQueue` is hypothetically slow in cleaning up finished tasks?
The two things that will hang around for a while
The completed K8s job/pod. I don't think this is a huge issue since its not actually consuming any resources (basically a key/value entry in etcd).
The entry in the tasks map. The API call for listing tasks will still return this value and some of the task slot metrics will report the task as still running. New tasks will still be able to be run since the thread executing the future for the task will have completed.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Move some lifecycle management from doTask -> shutdown for the mm-less task runner (druid)
Posted by "dclim (via GitHub)" <gi...@apache.org>.
dclim merged PR #14895:
URL: https://github.com/apache/druid/pull/14895
--
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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org