feat: Add new TaskLockType KILL for embedded kill tasks by kfaraz · Pull Request #19921 · apache/druid

kfaraz · 2026-08-07T17:17:20Z

Description

This has been split from #19772 .

Instead of acquiring a lock for markAsUsed and markAsUnused APIs (as it may potentially hamper ingestion),
this patch tries to relax the locking requirements of a kill task and adds restrictions on the window for markAsUsed.

Changes

Add new TaskLockType.KILL
This lock can coexist with any other lock type but not with another KILL lock to ensure that only a single kill task is working on a single datasource-interval
This lock type can currently be used only by embedded kill tasks so that the UnusedSegmentsKiller does not skip intervals with active ingestion.
Inject the SegmentsMetadataManagerConfig into SqlSegmentsMetadataQuery.
Do not allow a segment to be marked as used if it was last updated earlier than the buffer period. This ensures that an unused segment that is now eligible for kill is not accidentally marked as used while a kill task for that segment is in progress.
Add methods in SegmentMetadataTransactionFactory to provide read-write transactions which do not use the segment metadata cache. This moves away the transaction creation logic from IndexerSQLMetadataStorageCoordinator to the transaction factory and reduces the number of call sites that create a SqlSegmentsMetadataQuery.
Add tests

This PR has:

Introduces a new KILL lock type that can coexist with all other lock types (EXCLUSIVE, SHARED, REPLACE, APPEND) but not with another KILL lock on an overlapping interval. Only tasks of type "kill" are permitted to acquire a KILL lock. UnusedSegmentsKiller's embedded kill task now uses KILL locking by default, allowing concurrent ingest tasks to proceed while unused segments are being deleted. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Tests verify that a KILL lock can be acquired, released, and revoked for tasks that are never inserted into TaskStorage (the embedded kill task pattern used by UnusedSegmentsKiller). Specifically: - Acquire + release without task in storage unblocks the next acquirer - Lock is not restored after syncFromStorage (task not persisted) - Higher-priority kill task not in storage can revoke a lower-priority one - KILL lock coexists with other lock types held by persisted tasks Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Add tests to SqlSegmentsMetadataQueryTest covering the validateSegmentsForMarkingAsUsed guard: - throws CONFLICT when kill is enabled and segment was updated before buffer period - succeeds when kill is enabled but segment was updated within buffer period - succeeds when kill is disabled regardless of how old the update time is - covers markSegmentAsUsed, markNonOvershadowedSegmentsAsUsed, and markAllNonOvershadowedSegmentsAsUsed Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

FrankChen021

Severity	Findings
P0	0
P1	0
P2	1
P3	0
Total	1

Reviewed 35 of 35 changed files.

This is an automated review by Codex GPT-5.6-Luna(max)

FrankChen021 · 2026-08-10T12:54:49Z

-          Map.of(Tasks.PRIORITY_KEY, Tasks.DEFAULT_EMBEDDED_KILL_TASK_PRIORITY),
+          Map.of(
+              Tasks.PRIORITY_KEY, Tasks.DEFAULT_EMBEDDED_KILL_TASK_PRIORITY,
+              Tasks.TASK_LOCK_TYPE, TaskLockType.KILL.name()


[P2] KILL locks incorrectly suppress compaction

The compaction duty reports active locks through getLockedIntervals, which includes this priority-25 KILL lock even though KILL is designed to coexist with compaction locks. Compatible intervals are consequently deferred while a kill task is active. Make lock reporting honor lock compatibility and add a regression test.

Good catch! Addressed.

Confirmed fixed at this head: getLockedIntervals now ignores KILL locks for non-KILL requests, with regression coverage. I reviewed all 37 changed files and found no additional issues.

capistrant · 2026-08-10T14:54:38Z

+      throw DruidException.forPersona(DruidException.Persona.OPERATOR)
+                          .ofCategory(DruidException.Category.CONFLICT)
+                          .build(
+                              "Segment IDs[%s] cannot be marked as used since"
+                              + " they were last updated more than [%s] ago and"
+                              + " are now eligible for permanent deletion.",
+                              expiredSegmentIds, bufferPeriod
+                          );


does this new restriction need to be in docs? perhaps in the properties table that describes what the buffer period is and what happens after the segment ages out of the buffer?

documenting depends a lot on the outcome of my comment above related to only enforcing this when using embedded kill

Hmm, I think we should document it either way (at least for embedded kill tasks).

capistrant · 2026-08-10T15:18:45Z

+    if (!managerConfig.getKillUnused().isEnabled()) {
+      // Do not verify the buffer period if embedded kill tasks are not enabled
+      return;
+    }


hmmm. do we really need to introduce this exception to the rule? Having the whole system opted into the new restriction about how long a segment can be unused before not being eligible to mark used seems less sconfusing

Yeah, I started with that. But then I realized that the bufferPeriod config makes sense only when embedded kill is enabled, since the UnusedSegmentKillerConfig.bufferPeriod does not apply to regular kill tasks.

capistrant · 2026-08-10T22:22:42Z

        throw new ISE("Unable to grant LockPosse to inactive Task [%s]", task.getId());
      }

+      if (request.getType() == TaskLockType.KILL && !KillUnusedSegmentsTask.TYPE.equals(task.getType())) {


so manually submitted kill tasks can use the kill lock type, as far as I can tell at least. is that acceptable?

Yeah, this is on purpose but I see the problem. If we wanted to use the KILL lock type for manual kill tasks, we would also need to do the validation on mark-as-used even if embedded kill was disabled.

I don't see a clear way to do that, except maybe always honoring the bufferPeriod.

Updated this to allow KILL locks only for embedded kill tasks.

kfaraz · 2026-08-11T06:26:47Z

Thanks for the review, @capistrant !
I decided to use the new KILL lock type only for embedded kill tasks and also do the markAsUsed validation only when embedded kill is enabled.
This would be an additional incentive for users to try out the embedded kill tasks.

capistrant

thanks for responses to my questions and comments. looks good to me.

FrankChen021

I have reviewed the code for correctness, edge cases, concurrency, and integration risks; no issues found.

Reviewed 38 of 38 changed files.

This is an automated review by Codex GPT-5.6-Luna(max)

kfaraz and others added 2 commits August 7, 2026 13:20

Do not allow marking as used if segment ID has expired

5f1831f

github-actions Bot added the Area - Ingestion label Aug 7, 2026

kfaraz and others added 3 commits August 7, 2026 22:51

Add minor check

dc1efbe

Fix compilation

8a16166

github-advanced-security Bot found potential problems Aug 9, 2026

View reviewed changes

Comment thread indexing-service/src/test/java/org/apache/druid/indexing/overlord/GlobalTaskLockboxTest.java Fixed

kfaraz and others added 3 commits August 9, 2026 14:52

Clean up tests

2655e17

Fix some tests

b0d0fb5

kfaraz marked this pull request as ready for review August 9, 2026 14:51

kfaraz requested a review from gianm August 9, 2026 14:51

kfaraz added 2 commits August 9, 2026 21:00

Fix up some tests

5a7f7fd

Update docs

8c07d97

github-actions Bot added the Area - Documentation label Aug 9, 2026

kfaraz mentioned this pull request Aug 9, 2026

perf: Improve efficiency of embedded kill tasks #19772

Merged

10 tasks

kfaraz added 4 commits August 10, 2026 11:01

Fix tests some more

bce3f75

Fix checkstyle

eb275d3

Fix test exception

3cde058

Fix tests

774b5ec

FrankChen021 reviewed Aug 10, 2026

View reviewed changes

Address feedback, fix tests

d2117f4

capistrant reviewed Aug 10, 2026

View reviewed changes

Fixes

5509f47

kfaraz added 2 commits August 11, 2026 18:08

Merge branch 'master' of github.com:apache/druid into no_lock_for_kill

2d7657c

Fix tests yet again

14236d2

capistrant approved these changes Aug 11, 2026

View reviewed changes

Reduce kill buffer period in KafkaClusterMetricsTest

2bcfb56

FrankChen021 reviewed Aug 12, 2026

View reviewed changes

Fix KafkaClusterMetricsTest, kill intervals are not skipped anymore

ae3f7fc

kfaraz merged commit 978de63 into apache:master Aug 12, 2026
45 of 46 checks passed

kfaraz deleted the no_lock_for_kill branch August 12, 2026 16:21

github-actions Bot added this to the 39.0.0 milestone Aug 12, 2026

Uh oh!

Conversation

kfaraz commented Aug 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Changes

Uh oh!

Uh oh!

FrankChen021 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kfaraz commented Aug 11, 2026

Uh oh!

capistrant left a comment

Choose a reason for hiding this comment

Uh oh!

FrankChen021 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kfaraz commented Aug 7, 2026 •

edited

Loading