You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "kfaraz (via GitHub)" <gi...@apache.org> on 2025/09/26 04:18:12 UTC
[PR] Make KubernetesWorkItem.shutdown idempotent (druid)
kfaraz opened a new pull request, #18576:
URL: https://github.com/apache/druid/pull/18576
### Description
This patch tries to address the following condition due to buggy fabric8 client.
The callback executor in `TaskQueue` can get stuck in the following scenario.
- `TaskQueue` tries to shutdown a task via `notifyStatus()` and queues up a `TaskRunner.shutdown()` on the callback executor.
- `TaskQueue` then removes the entry for this task from its in-memory data-structures
- `TaskRunner.shutdown()` remains stuck due to the fabric8 bug
- `TaskRunner` still has the task in its memory
- When the `TaskQueue` periodically manages itself via `startPendingTasksOnRunner()`,
it realizes that the `TaskRunner` has this unknown task and tries to shut it down again.
### Fix
Subsequent calls to `KubernetesWorkItem.shutdown()` should not block and return immediately.
<hr>
This PR has:
- [ ] 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.
- [ ] 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.
- [ ] 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] Make KubernetesWorkItem.shutdown idempotent (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #18576:
URL: https://github.com/apache/druid/pull/18576#discussion_r2382747853
##########
extensions-core/kubernetes-overlord-extensions/src/main/java/org/apache/druid/k8s/overlord/KubernetesWorkItem.java:
##########
@@ -29,23 +29,33 @@
import org.apache.druid.java.util.common.ISE;
import java.io.InputStream;
+import java.util.concurrent.atomic.AtomicBoolean;
public class KubernetesWorkItem extends TaskRunnerWorkItem
{
private final Task task;
private final KubernetesPeonLifecycle kubernetesPeonLifecycle;
+ private final AtomicBoolean isShutdown = new AtomicBoolean(false);
+
public KubernetesWorkItem(Task task, ListenableFuture<TaskStatus> statusFuture, KubernetesPeonLifecycle kubernetesPeonLifecycle)
{
super(task.getId(), statusFuture);
this.task = task;
this.kubernetesPeonLifecycle = kubernetesPeonLifecycle;
}
- protected synchronized void shutdown()
+ /**
+ * Shuts down this work item. Subsequent calls to this method return immediately.
+ */
+ protected void shutdown()
{
- this.kubernetesPeonLifecycle.startWatchingLogs();
- this.kubernetesPeonLifecycle.shutdown();
+ if (isShutdown.compareAndSet(false, true)) {
+ synchronized (this) {
+ this.kubernetesPeonLifecycle.startWatchingLogs();
Review Comment:
Yeah, I agree, the only apparent effect of calling `startWatchingLogs()` is to initialize `KubernetesPeonLifecycle.logWatch` field which would be initialized anyway via `join()` -> `finally` -> `saveLogs()` -> `doSaveLogs()`.
We can explore that in a separate PR.
--
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] Make KubernetesWorkItem.shutdown idempotent (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz merged PR #18576:
URL: https://github.com/apache/druid/pull/18576
--
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] Make KubernetesWorkItem.shutdown idempotent (druid)
Posted by "capistrant (via GitHub)" <gi...@apache.org>.
capistrant commented on code in PR #18576:
URL: https://github.com/apache/druid/pull/18576#discussion_r2382648985
##########
extensions-core/kubernetes-overlord-extensions/src/main/java/org/apache/druid/k8s/overlord/KubernetesWorkItem.java:
##########
@@ -29,23 +29,33 @@
import org.apache.druid.java.util.common.ISE;
import java.io.InputStream;
+import java.util.concurrent.atomic.AtomicBoolean;
public class KubernetesWorkItem extends TaskRunnerWorkItem
{
private final Task task;
private final KubernetesPeonLifecycle kubernetesPeonLifecycle;
+ private final AtomicBoolean isShutdown = new AtomicBoolean(false);
+
public KubernetesWorkItem(Task task, ListenableFuture<TaskStatus> statusFuture, KubernetesPeonLifecycle kubernetesPeonLifecycle)
{
super(task.getId(), statusFuture);
this.task = task;
this.kubernetesPeonLifecycle = kubernetesPeonLifecycle;
}
- protected synchronized void shutdown()
+ /**
+ * Shuts down this work item. Subsequent calls to this method return immediately.
+ */
+ protected void shutdown()
{
- this.kubernetesPeonLifecycle.startWatchingLogs();
- this.kubernetesPeonLifecycle.shutdown();
+ if (isShutdown.compareAndSet(false, true)) {
+ synchronized (this) {
+ this.kubernetesPeonLifecycle.startWatchingLogs();
Review Comment:
do we even need to be calling `startWatchingLogs()` here? Kind of off topic for the PR. But part of the reason shutdown is slow and can even hang, is because of the LogWatch being unhealthy. https://github.com/apache/druid/pull/18444 was implemented to put a time limit on log persist for saving off task logs. In that code path we are actually writing the logs out somewhere. I guess I don't see how the call here is useful at all if we are trying to just shut the task down?
--
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] Make KubernetesWorkItem.shutdown idempotent (druid)
Posted by "capistrant (via GitHub)" <gi...@apache.org>.
capistrant commented on PR #18576:
URL: https://github.com/apache/druid/pull/18576#issuecomment-3339531928
https://github.com/apache/druid/pull/18579 opened this for proposing dropping the LogWatch init in shutdown
--
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