Allow for task limit on kill tasks spawned by auto kill coordinator duty #14769

zachjsh · 2023-08-07T18:21:21Z

Description

Previously, the KillUnusedSegments coordinator duty, in charge of periodically deleting unused segments, could spawn an unlimited number of kill tasks for unused segments. This change adds 2 new coordinator dynamic configs that can be used to control the limit of tasks spawned by this coordinator duty

killTaskSlotRatio: Ratio of total available task slots, including autoscaling if applicable that will be allowed for kill tasks. This limit only applies for kill tasks that are spawned automatically by the coordinator's auto kill duty. Default is 1, which allows all available tasks to be used, which is the existing behavior

maxKillTaskSlots: Maximum number of tasks that will be allowed for kill tasks. This limit only applies for kill tasks that are spawned automatically by the coordinator's auto kill duty. Default is INT.MAX, which essentially allows for unbounded number of tasks, which is the existing behavior.

Realize that we can effectively get away with just the one killTaskSlotRatio, but following similarly to the compaction config, which has similar properties; I thought it was good to have some control of the upper limit regardless of ratio provided.

Release note

NEW: killTaskSlotRatio and maxKillTaskSlots coordinator dynamic config properties added that allow control of task resource usage spawned by KillUnusedSegments coordinator task (auto kill)

This PR has:

Previously, the `KillUnusedSegments` coordinator duty, in charge of periodically deleting unused segments, could spawn an unlimited number of kill tasks for unused segments. This change adds 2 new coordinator dynamic configs that can be used to control the limit of tasks spawned by this coordinator duty

jasonk000

Generally LGTM! Some tweaks to consider.

jasonk000 · 2023-08-07T20:16:54Z

server/src/main/java/org/apache/druid/server/coordinator/duty/KillUnusedSegments.java

@@ -149,7 +173,7 @@ private void killUnusedSegments(Collection<String> dataSourcesToKill)
      }
    }

-    log.debug("Submitted kill tasks for [%d] datasources.", submittedTasks);
+    log.debug("Submitted [%d] kill tasks for [%d] datasources.", submittedTasks, dataSourcesToKill.size());


if any datasources were skipped due to not enough slots, we could consider logging them here?

Will get this in a follow up patch

jasonk000 · 2023-08-07T20:20:29Z

server/src/main/java/org/apache/druid/server/coordinator/duty/KillUnusedSegments.java

+      while (activeTasks.hasNext()) {
+        final TaskStatusPlus status = activeTasks.next();
+
+        // taskType can be null if middleManagers are running with an older version. Here, we consevatively regard


I think this is important enough to capture as a method-level javadoc hint. ie: this might overestimate in some circumstances.

taskType can be null if middleManagers are running with an older version.

Do we know in which versio the middle managers would report the task type as null? I saw the same comment in the compact segments duty that was from mid 2018. So I wonder if this null check is that important

Thats exactly where I got it from 😆 . Just wanted to play it safe.

jasonk000 · 2023-08-07T20:24:51Z

server/src/main/java/org/apache/druid/server/coordinator/duty/KillUnusedSegments.java

+    return numActiveKillTasks;
+  }
+
+  private int getKillTaskCapacity(double killTaskSlotRatio, int maxKillTaskSlots)


Personally, I'd split this to a getTotalWorkerCapacity, to keep the kill calculation separate. I think testing would be cleaner this way too

It would be great to see a unit test that explicitly could test getKillTaskCapacity based only on numeric inputs, so that you can test 0s, infinities, etc.

Thanks! Fixed, and added tests. Let me know if better now.

jasonk000 · 2023-08-07T20:26:04Z

server/src/test/java/org/apache/druid/server/coordinator/duty/KillUnusedSegmentsTest.java

+  public void testNoAvailableTaskCapacityForKill()
+  {
+    mockNoAvailableTaskSlotsForKill();
+    runAndVerifyNoKill();


* address review comments

suneet-s

Nice improvement!

server/src/main/java/org/apache/druid/server/coordinator/CoordinatorDynamicConfig.java

suneet-s · 2023-08-08T02:44:18Z

server/src/main/java/org/apache/druid/server/coordinator/CoordinatorDynamicConfig.java

@@ -495,6 +533,8 @@ private static class Defaults
    static final int MAX_NON_PRIMARY_REPLICANTS_TO_LOAD = Integer.MAX_VALUE;
    static final boolean USE_ROUND_ROBIN_ASSIGNMENT = true;
    static final boolean SMART_SEGMENT_LOADING = true;
+    static final double KILL_TASK_SLOT_RATIO = 1.0;
+    static final int MAX_KILL_TASK_SLOTS = Integer.MAX_VALUE;


A comment here that these defaults are to preserve the behavior before Druid 0.28 and a future version may want to consider better defaults so that kill tasks can not eat up all the capacity in the cluster would be nice

added, thanks

suneet-s · 2023-08-08T02:49:15Z

server/src/main/java/org/apache/druid/server/coordinator/duty/KillUnusedSegments.java

+    if (0 == availableKillTaskSlots) {
+      log.warn("Not killing any unused segments because there are no available kill task slots at this time.");


Does this need to be a warning? It seems like it could be quite verbose if there are long running kill tasks and the KillUnusedSegments duty is scheduled to run frequently.

Perhaps, a better way of surfacing this information is emitting the number of kill tasks and calculated kill capacity as metrics so that operators can look at these metrics to decide if they need more kill capacity.

changed to debug. 💯 about adding metrics, will do that in a follow up patch.

suneet-s · 2023-08-08T02:52:39Z

server/src/main/java/org/apache/druid/server/coordinator/duty/KillUnusedSegments.java

+      while (activeTasks.hasNext()) {
+        final TaskStatusPlus status = activeTasks.next();
+
+        // taskType can be null if middleManagers are running with an older version. Here, we consevatively regard


taskType can be null if middleManagers are running with an older version.

Do we know in which versio the middle managers would report the task type as null? I saw the same comment in the compact segments duty that was from mid 2018. So I wonder if this null check is that important

suneet-s · 2023-08-08T02:57:44Z

server/src/main/java/org/apache/druid/server/coordinator/duty/KillUnusedSegments.java

+    return numActiveKillTasks;
+  }
+
+  private int getTotalWorkerCapacity()


It seems like a lot of this code is very similar to the code in CompactSegments Can we refactor the code so that the code can be re-used in both duties?

Yes absolutely correct. Will refactor this in a follow up patch.

suneet-s

🚀 Looking forward to the follow up patches!

zachjsh requested a review from suneet-s August 7, 2023 18:21

github-actions bot added the Area - Documentation label Aug 7, 2023

zachjsh added the Area - Operations label Aug 7, 2023

jasonk000 approved these changes Aug 7, 2023

View reviewed changes

* fix bug in propogation of new config values in builder

6b623bd

* address review comments

suneet-s reviewed Aug 8, 2023

View reviewed changes

* address review comments

443d491

zachjsh requested a review from suneet-s August 8, 2023 04:58

* fix logging bug

1de4f3e

suneet-s approved these changes Aug 8, 2023

View reviewed changes

zachjsh added 2 commits August 8, 2023 01:19

* log bug fix

9ba0bee

* fix logging once and for all

da4c5c2

zachjsh merged commit 660e6cf into apache:master Aug 8, 2023
74 checks passed

zachjsh deleted the auto-kill-resource-control branch August 8, 2023 12:40

vogievetsky added the Needs web console change Backend API changes that would benefit from frontend support in the web console label Aug 8, 2023

vogievetsky mentioned this pull request Aug 9, 2023

Web console: expose new coordinator properties in the dialog #14791

Merged

vogievetsky removed the Needs web console change Backend API changes that would benefit from frontend support in the web console label Aug 17, 2023

LakshSingla added this to the 28.0 milestone Oct 12, 2023

LakshSingla mentioned this pull request Nov 4, 2023

[DRAFT] 28.0.0 release notes #15326

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow for task limit on kill tasks spawned by auto kill coordinator duty #14769

Allow for task limit on kill tasks spawned by auto kill coordinator duty #14769

zachjsh commented Aug 7, 2023 •

edited

Loading

jasonk000 left a comment

jasonk000 Aug 7, 2023

zachjsh Aug 8, 2023

jasonk000 Aug 7, 2023

suneet-s Aug 8, 2023

zachjsh Aug 8, 2023

jasonk000 Aug 7, 2023

zachjsh Aug 7, 2023

jasonk000 Aug 7, 2023

suneet-s left a comment

suneet-s Aug 8, 2023 •

edited

Loading

zachjsh Aug 8, 2023

suneet-s Aug 8, 2023

zachjsh Aug 8, 2023

suneet-s Aug 8, 2023

suneet-s Aug 8, 2023

zachjsh Aug 8, 2023

suneet-s left a comment

		if (0 == availableKillTaskSlots) {
		log.warn("Not killing any unused segments because there are no available kill task slots at this time.");

Allow for task limit on kill tasks spawned by auto kill coordinator duty #14769

Allow for task limit on kill tasks spawned by auto kill coordinator duty #14769

Conversation

zachjsh commented Aug 7, 2023 • edited Loading

Description

Release note

jasonk000 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

suneet-s left a comment

Choose a reason for hiding this comment

suneet-s Aug 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

suneet-s left a comment

Choose a reason for hiding this comment

zachjsh commented Aug 7, 2023 •

edited

Loading

suneet-s Aug 8, 2023 •

edited

Loading