Set default `KillUnusedSegments` duty to coordinator's indexing period & `killTaskSlotRatio` to 0.1 #16247

abhishekrb19 · 2024-04-08T21:32:33Z

Problem:

When druid.coordinator.kill.period is set to something other than the coordinator's indexing period, it can cause confusion regarding when the duty will run. Please see #15911 for example. Also, the default P1D may be overly conservative for most users as noted in this comment.

Separately, the default value of the dynamic coordinator config killTaskSlotRatio was previously 1.0, which means virtually all available slots in a cluster can be used by the KillUnusedSegments duty, starving any other tasks in the system.

Fix:

Fixes #15911. With sufficient guard rails available in the kill duty, the duty can just run regularly at the same time as the coordinator's indexing period.

By default, when druid.coordinator.kill.period is not set explicitly, it will take the value of druid.coordinator.period.indexingPeriod (whether it's the default or an overridden value)
As before, an operator can choose to override druid.coordinator.kill.period and this will take precedence over the default behavior noted above.
Updated default value of killTaskSlotRatio to 0.1. This means with the default dynamic coordinator config, the KillUnusedSegments duty will at most take only 10% of the available cluster capacity task slots.

We have been successfully running the auto-kill duty in production with druid.coordinator.kill.period set to druid.coordinator.period.indexingPeriod, even at PT15M. Note that we could deprecate druid.coordinator.kill.period, but in the ethos of providing flexibility to operators, this patch doesn't do so.

Other related changes

The eternity interval comment is inaccurate and a remnant of Fixup KillUnusedSegmentsTest #16094, so removed the relevant javadoc
Fix typo in CoordinatorCompactionConfig
Break up the tests in DruidCoordinatorConfigTest

Release note

The default value for druid.coordinator.kill.period (if unspecified) has changed from P1D to the value of druid.coordinator.period.indexingPeriod. Operators can choose to override druid.coordinator.kill.period and that will take precedence over the default behavior.
The default value for the coordinator dynamic config killTaskSlotRatio is updated from 1.0 to 0.1. This ensures that that kill tasks take up only 1 task slot right out-of-the-box instead of taking up all the task slots.

This PR has:

been self-reviewed.
added documentation for new or modified features or behaviors.
a release note entry in the PR description.
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 is met.
been tested in a test Druid cluster.

…dexingPeriod if not set. - Remove the default P1D value for druid.coordinator.kill.period. Instead default druid.coordinator.kill.period to whatever value druid.coordinator.period.indexingPeriod is set to if the former config isn't specified. - If druid.coordinator.kill.period is set, the value will take precedence over druid.coordinator.period.indexingPeriod

server/src/test/java/org/apache/druid/server/coordinator/DruidCoordinatorConfigTest.java

…CoordinatorConfigTest.java

…cubator-druid into revisit_auto_kill_period

kfaraz

@abhishekrb19 , it makes sense to update default values of configs to improve out-of-the-box Druid behaviour. But I have some concerns.

After this change, the default druid.coordinator.kill.period effectively changes from 1 day to 30 mins (default value of indexing period). So potentially, there could be a kill task submitted for every datasource every 30 mins. I wonder if there can be clusters for which this is harmful / wasteful in terms of task resources.

Clusters which need frequent cleanup would have already set a low kill.period. But now other clusters which didn't want frequent cleanup (due to limited task resources) would have to go and update their configs.

For the linked issue #15911 , have we been able to debug why the kill.period now seems to be offset by the indexingPeriod? That seems like a bug rather than an issue with the default value.

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

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

…CoordinatorConfig.java Co-authored-by: Kashif Faraz <kashif.faraz@gmail.com>

abhishekrb19 · 2024-04-09T04:42:11Z

@kfaraz - thanks for the swift review!

Re:

After this change, the default druid.coordinator.kill.period effectively changes from 1 day to 30 mins (default value of indexing period). So potentially, there could be a kill task submitted for every datasource every 30 mins. I wonder if there can be clusters for which this is harmful / wasteful in terms of task resources.

I was thinking of changing the default killTaskSlotRatio from 1 to 0.1, so the number of kill tasks spun will be bound, preventing starvation of other tasks. This will also be in alignment with compaction task slot ratio.

Upon reviewing the reasoning in the code here for the current default values of kill task slots, it appears that the unbounded values were chosen to preserve old behavior, which I'd argue was not ideal to start off with. Given that we're changing the kill period's default in this patch, we might as well take an opportunity to adjust adjacent kill configurations to set them to safe values for a better out-of-the-box experience, and highlight the changes in the release notes. What do you think?

abhishekrb19 · 2024-04-09T04:52:53Z

Clusters which need frequent cleanup would have already set a low kill.period. But now other clusters which didn't want frequent cleanup (due to limited task resources) would have to go and update their configs.

Just as above, would setting the default value of killTaskSlotRatio to 0.1 or a finite value should address this concern? If an operator prefers to run the kill duty at a lower cadence after reviewing the release notes, they may choose to override the default behavior in their cluster.

For the linked issue #15911 , have we been able to debug why the kill.period now seems to be offset by the indexingPeriod? That seems like a bug rather than an issue with the default value.

Yes, there were more details in this community slack thread

server/src/test/java/org/apache/druid/server/coordinator/DruidCoordinatorConfigTest.java

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

kfaraz · 2024-04-10T03:38:13Z

Yes, @abhishekrb19 , I think reducing killTaskSlotRatio to 0.1 (with minimum kill task slots as 1, similar to the compaction slots logic) in this same PR would make sense.

…fault mock

abhishekrb19 · 2024-04-10T18:33:34Z

server/src/test/java/org/apache/druid/server/http/CoordinatorDynamicConfigTest.java

@@ -201,7 +197,7 @@ public void testSerde() throws Exception
        1,
        2,
        whitelist,
-        1.0,
+        0.1,


This value change from 1.0 to 0.1 is the default change of killTaskSlotRatio made in this patch.
The test should just use a builder to make it clear, or the assertion should happen in the tests, but that cleanup is for another day.

zachjsh

LGTM

abhishekrb19 added 2 commits April 8, 2024 13:00

Remove stale comment and inline canDutyRun()

b84e60a

github-actions bot added the Area - Documentation label Apr 8, 2024

abhishekrb19 added the Release Notes label Apr 8, 2024

abhishekrb19 requested review from zachjsh and kfaraz April 8, 2024 21:35

abhishekrb19 commented Apr 8, 2024

View reviewed changes

server/src/test/java/org/apache/druid/server/coordinator/DruidCoordinatorConfigTest.java Outdated Show resolved Hide resolved

abhishekrb19 added 4 commits April 8, 2024 14:36

Update server/src/test/java/org/apache/druid/server/coordinator/Druid…

c4283e2

…CoordinatorConfigTest.java

Fix checkstyle error

68249d7

Merge branch 'revisit_auto_kill_period' of github.com:abhishekrb19/in…

709f656

…cubator-druid into revisit_auto_kill_period

Clarify comment

1b9e6b8

kfaraz reviewed Apr 9, 2024

View reviewed changes

abhishekrb19 and others added 2 commits April 8, 2024 21:20

Update server/src/main/java/org/apache/druid/server/coordinator/Druid…

1ed6334

…CoordinatorConfig.java Co-authored-by: Kashif Faraz <kashif.faraz@gmail.com>

Put back canDutyRun()

a738cde

abhishekrb19 changed the title ~~Set default kill period to coordinator's indexing period~~ Set default kill unused segments period to coordinator's indexing period Apr 9, 2024

zachjsh reviewed Apr 9, 2024

View reviewed changes

server/src/test/java/org/apache/druid/server/coordinator/DruidCoordinatorConfigTest.java Show resolved Hide resolved

suneet-s reviewed Apr 9, 2024

View reviewed changes

server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinatorConfig.java Show resolved Hide resolved

abhishekrb19 added 5 commits April 10, 2024 07:48

Default killTaskSlotsRatio to 0.1 instead of 1.0 (all slots)

ee48f25

Fix typo DEFAULT_MAX_COMPACTION_TASK_SLOTS

2d584e7

Remove unused test method.

6224c9f

Update default value of killTaskSlotsRatio in docs and web-console de…

37af285

…fault mock

Merge branch 'master' into revisit_auto_kill_period

75e9813

github-actions bot added the Area - Web Console label Apr 10, 2024

Move initDuty() after params and config setup.

d62a61e

abhishekrb19 changed the title ~~Set default kill unused segments period to coordinator's indexing period~~ Set default KillUnusedSegments duty to coordinator's indexing period & killTaskSlots to 0.1 Apr 10, 2024

abhishekrb19 commented Apr 10, 2024

View reviewed changes

abhishekrb19 changed the title ~~Set default KillUnusedSegments duty to coordinator's indexing period & killTaskSlots to 0.1~~ Set default KillUnusedSegments duty to coordinator's indexing period & killTaskSlotRatio to 0.1 Apr 10, 2024

zachjsh approved these changes Apr 11, 2024

View reviewed changes

abhishekrb19 merged commit 041d0bf into apache:master Apr 15, 2024
85 checks passed

abhishekrb19 deleted the revisit_auto_kill_period branch April 15, 2024 01:56

adarshsanjeev added this to the 30.0.0 milestone May 6, 2024

adarshsanjeev mentioned this pull request May 28, 2024

[DRAFT] 30.0.0 release notes #16505

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set default `KillUnusedSegments` duty to coordinator's indexing period & `killTaskSlotRatio` to 0.1 #16247

Set default `KillUnusedSegments` duty to coordinator's indexing period & `killTaskSlotRatio` to 0.1 #16247

abhishekrb19 commented Apr 8, 2024 •

edited

Loading

kfaraz left a comment

abhishekrb19 commented Apr 9, 2024

abhishekrb19 commented Apr 9, 2024 •

edited

Loading

kfaraz commented Apr 10, 2024 •

edited

Loading

abhishekrb19 Apr 10, 2024

zachjsh left a comment

Set default KillUnusedSegments duty to coordinator's indexing period & killTaskSlotRatio to 0.1 #16247

Set default KillUnusedSegments duty to coordinator's indexing period & killTaskSlotRatio to 0.1 #16247

Conversation

abhishekrb19 commented Apr 8, 2024 • edited Loading

Problem:

Fix:

Other related changes

Release note

kfaraz left a comment

Choose a reason for hiding this comment

abhishekrb19 commented Apr 9, 2024

abhishekrb19 commented Apr 9, 2024 • edited Loading

kfaraz commented Apr 10, 2024 • edited Loading

abhishekrb19 Apr 10, 2024

Choose a reason for hiding this comment

zachjsh left a comment

Choose a reason for hiding this comment

Set default `KillUnusedSegments` duty to coordinator's indexing period & `killTaskSlotRatio` to 0.1 #16247

Set default `KillUnusedSegments` duty to coordinator's indexing period & `killTaskSlotRatio` to 0.1 #16247

abhishekrb19 commented Apr 8, 2024 •

edited

Loading

abhishekrb19 commented Apr 9, 2024 •

edited

Loading

kfaraz commented Apr 10, 2024 •

edited

Loading