perf: Improve efficiency of embedded kill tasks by kfaraz · Pull Request #19772 · apache/druid

kfaraz · 2026-07-27T18:51:54Z

Description

The UnusedSegmentsKiller currenly has some limitations:

Wasted cycles: The kill queue contains jobs for all intervals which have an unused segment, regardless of whether the unused segment is eligible for kill or not.
Slow progress: For intervals with a large number of killable unused segments, a single embedded kill task is launched which would kill only 1000 segments in one go. In such cases, the UnusedSegmentsKiller would take very long to clear a backlog of killable unused segments.
Many metadata queries: When the kill queue is rebuilt, heavy metadata queries are fired for each datasource in the DB, even if they don't have any unused segments, killable or otherwise.

Changes

While rebuilding the kill queue, fire a single metadata query to retrieve the intervals containing killable unused segments of all datasources.
- This avoids firing multiple queries, one for each datasource in the DB.
- It also ensures that every embedded kill task actually kills atleast 1 unused segment.
In the above query, scan upto a maximum of 200k eligible unused segments. This would also be the maximum number of segments killed when the queue has been fully processed.
- This number is large enough to make meaningful progress in each cycle of the UnusedSegmentsKiller and small enough to keep the query time within ~5s (tried on MySQL 8 with 17M unused segments).
Pass in the max segments to kill in each KillCandidate.
Allow each embedded kill task to kill multiple batches of segments, upto a maximum of 10 (previously 1).

This PR has:

cryptoe

+1
I think the changes make sense to me.
Thank you for working on this.

cryptoe · 2026-07-27T19:42:22Z

+      // Identify intervals with unused segments which are eligible for kill
+      final Map<DatasourceInterval, Integer> killCandidates =
+          storageCoordinator.retrieveSomeUnusedSegmentIntervals(
+              DateTimes.nowUtc().minus(killConfig.getBufferPeriod()),


So we have basically pushed in the bufferPeriod inside the sql query so that we donot populate segments which would be ineligible.
I think this change alone would get us lot of improvements.
I am a bit worried about fetching all the data sources at once but I guess there is where the inner query limit comes into place.

Yes, the inner limit (currently 200k) takes care of that.

Fetching all the datasources in one go is actually better since in the current flow, we would fire separate queries for each datasource. When the number of datasources is large, this can be pretty redundant, especially since a lot of the datasources might not even have killable unused segments.

gianm · 2026-07-27T22:26:25Z

-  @Test
-  public void test_maxSegmentsKilledInAnInterval_is_1k()
+  @Test(timeout = 30_000L)
+  public void test_maxSegmentsKilledInRun_is_200k()


On my machine this test legitimately takes nearly 30s to run, which has two problems:

The timeout = 30_000L is low enough that it would make the test flaky

30s is just generally a long time.

Is it possible to change the test to run faster while still being effective?

Added a config to control the maxSegmentsToKill.
This allows us to use lower values for tests and for any cluster where a higher/lower value might be preferable.
Although, most clusters should work fine with the default value of 200k itself.

FrankChen021

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

The capped global scan can starve cleanup behind locked intervals, embedded tasks can exceed their discovered deletion limits, and malformed interval rows can abort queue rebuilding.

Reviewed 8 of 8 changed files.

This is an automated review by Codex GPT-5.6-Sol

FrankChen021 · 2026-07-28T12:12:45Z

+        //CHECKSTYLE.ON: Regexp
+        dbTables.getSegmentsTable(),
+        connector.getQuoteString(),
+        connector.limitClause(maxSegmentsToScan),


[P1] Prevent locked rows from monopolizing the global scan

The new global LIMIT has no ordering, cursor, or rotation state. If the selected 200,000 rows belong to intervals whose exclusive locks cannot be acquired, all generated tasks are skipped, the rows remain, and the next rebuild can select the identical slice again. Eligible segments outside that slice—including unrelated datasources—can then be starved indefinitely, whereas the previous per-datasource discovery still allowed other datasources to progress. Candidate discovery needs a deterministic progress/fairness mechanism that can advance past repeatedly skipped intervals.

While this is possible, it is not very likely to happen.

Firstly, intervals will not remain locked forever, unless there is a lot of late arriving data in streaming supervisors.

Secondly, the search query need not pick up the same intervals in the next iteration.

Also, the eventual goal is to use concurrent locks in embedded kill tasks, thereby removing the need to skip locked intervals altogether. I will create a follow up PR for that change.

Thanks for the context. After rechecking all 10 of 10 changed files, I would retain this finding. With no ordering, cursor, or rotation state, a stable query plan can return the same physical prefix on each rebuild. The locks need not be literally permanent: ongoing late-data activity can keep a prefix of streaming intervals locked while every row outside the global cap, including unrelated datasources, remains undiscovered. Since concurrent locking is deferred, this PR still needs deterministic progress or fairness.

cryptoe

LGTM post the doc changes for the new metric ELIGIBLE_UNUSED_SEGMENTS

cryptoe · 2026-07-29T14:49:39Z

+   * The default for embedded kill tasks may differ from the cluster-level default
+   * {@link Tasks#DEFAULT_USE_CONCURRENT_LOCKS}.
+   */
+  private static final boolean DEFAULT_USE_CONCURRENT_LOCKS = true;


Maybe add a comment here why this is safe for future readers.

Done. Added info at the class-level javadoc and linked to it from the constant javadoc.

kfaraz · 2026-07-30T06:54:23Z

Thanks for the reviews, @cryptoe ! I have added docs for the new metric.

FrankChen021

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

Reviewed 22 of 22 changed files.

Validation: git diff --check passed.

This is an automated review by Codex GPT-5.6-Sol

FrankChen021 · 2026-07-30T12:41:19Z

          null,
+          Map.of(
+              Tasks.PRIORITY_KEY, Tasks.DEFAULT_EMBEDDED_KILL_TASK_PRIORITY,
+              Tasks.USE_CONCURRENT_LOCKS, useConcurrentLocks


[P1] REPLACE locks do not resolve capped-scan starvation

REPLACE only coexists with APPEND. Default ingestion still uses EXCLUSIVE locks, and SHARED or REPLACE jobs also remain incompatible. Those intervals are skipped and selected again on every capped scan, indefinitely starving eligible intervals beyond the cap. The prior head-of-line finding remains unresolved unless queue rebuilding advances past skipped candidates.

This is okay, it is a nudge towards using concurrent locks.

I don't think REPLACE alone closes this issue. Ingestion defaults to useConcurrentLocks=false (EXCLUSIVE), and REPLACE remains incompatible with EXCLUSIVE, SHARED, and REPLACE locks. Since skipped candidates are discarded and the capped SQL subquery has neither ordering nor a cursor, the same long-lived locked rows can be returned on every rebuild and keep rows beyond maxSegmentsToKill from ever entering the queue. Please make the scan advance past skipped candidates or otherwise guarantee progress.

Reviewed 29 of 29 changed files.

gianm · 2026-07-30T14:52:30Z

  private static final String TASK_ID_PREFIX = "overlord-issued";

+  /**
+   * Use concurrent locks by default.


In delete.md there is this text added in #19737:

When using concurrent locks to run a kill task, ensure to keep a large enough buffer period before killing segments after they have been marked as unused. Otherwise, there may be a potential data loss if a concurrent append job upgrades one of the segments that are being killed.

There is a similar note in the javadoc for KillUnusedSegmentsTask. Why don't we need to worry about that here? Can the rationale be added here, and possibly referenced in the other places?

The older comment added in #19737 was somewhat incorrect.

The only case where a kill task could cause a potential data loss was if something like this happened:

kill task starts and identifies the unused segments to kill

markAsUsed API marks some of the target unused segments as used

a concurrent APPEND job upgrades some of these segments

kill task still thinks that the segments are unused and hard deletes their metadata entry as well as deep storage files

This patch originally included a javadoc comment explaining the above.

But then I decided that we might as well address the underlying issue, i.e. make the markAsUsed API mutually exclusive with any indexing/kill task. The docs already mention that calling these APIs while a task is in progress may lead to erratic behaviour.

Please let me know what you think.

FrankChen021

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

Reviewed 29 of 29 changed files.

This is an automated review by Codex GPT-5.6-Sol

FrankChen021 · 2026-07-31T13:33:49Z

+  {
+    public ExclusiveIntervalDummyTask(String id, String dataSource, Interval interval)
+    {
+      super(id, dataSource, interval, Map.of());


[P2] Prevent the lock probe from revoking active tasks

ExclusiveIntervalDummyTask has an empty context, so it requests its EXCLUSIVE lock at priority 0. TaskLockbox.tryLock revokes incompatible locks below the requester's priority before granting the lock. A mark-used API call can therefore abort a negative-priority ingestion task and succeed instead of returning the documented conflict response whenever another task is active. Use minimum priority or a non-preempting acquisition path so any existing incompatible lock makes this probe fail.

Default task priority is 0, so the markUsed API will get 0 priority.
Batch, realtime, and compact tasks have higher default priorities.

Agreed that the built-in ingestion defaults are above 0. However, task priority is explicitly user-overridable through the task context, and task submission does not enforce a lower bound. An ingestion task configured with "priority": -1 therefore holds a lock below this dummy task's priority 0, causing revokeAllIncompatibleActiveLocksIfPossible to revoke it. Since this API is intended to return a conflict whenever another task holds an incompatible lock, the probe still needs a minimum priority or a non-preempting acquisition path.

Reviewed 3 of 29 changed files.

gianm

The change to the metadata query looks good to me. The locking changes, I'm not as sure about. I left a few comments.

Is it possible to split the PRs out, so they can be reviewed independently? I don't believe they necessarily need to be connected, and the metadata query changes should be good to merge more or less as-is.

gianm · 2026-08-03T21:23:51Z

 - Coordinator APIs for data management are now deprecated. Use new APIs served by the Overlord instead.
- Do not use these APIs while an indexing task or kill task is in progress for the same datasource and interval.
+- The APIs to mark segments as used fail if an indexing task or kill task is in progress for the same datasource and overlapping interval, to ensure that there are no accidental data losses or data inconsistencies.
+- Do not use the APIs to mark segments as unused while an indexing task or kill task is in progress for the same datasource and interval.


Why the asymmetry? How are people supposed to know if an indexing or kill task is in progress? (It isn't obvious what the interval is without looking at the specs, which is tedious if a lot of tasks are running.)

Yes, I will try to include the info of the locked interval in the exception that gets thrown.

I will create a separate patch for the locking changes to markUsed as well as markUnused APIs.
The markUnused would be trickier as it is frequently used by the Coordinator to delete overshadowed segments. Acquiring an EXCLUSIVE lock while the Coordinator does this is not desirable.

locking changes removed from this PR.

gianm · 2026-08-06T06:38:55Z

+  private final Map<String, Object> context;
+
+  @JsonCreator
+  public DefaultTaskConfig(


Why this change? It seems like the end result would be similar.

Having the constructor simplifies instantiating this config in tests.

removed for now.

gianm · 2026-08-06T06:46:31Z

+   * map is not empty. However, it does NOT guarantee that:
+   * <ul>
+   * <li>the candidates in the returned map would be ordered by datasource or interval</li>
+   * <li>the result would contain {@code limit} entries when there are more distinct


limit is not a parameter. Is this meant to refer to maxResultSize?

gianm · 2026-08-06T06:47:46Z

  }

+  /**
+   * Scans upto {@code maxSegmentsToScan} unused segments which are eligible for


up to (spelling)

Also, this seems to be a copy of the javadoc for IndexerMetadataStorageCoordinator#retrieveSomeUnusedSegmentIntervals. Can one reference the other rather than copying the text?

gianm · 2026-08-06T07:31:26Z

+
+    try {
+      // Acquire lock on the interval before performing the update operation
+      taskLockbox.add(dummyTask);


It seems sketchy to insert a dummy task into the TaskLockbox. Certain operations in the lockbox require the task to exist in storage, such as revokeLock, which I think might get called if a higher-priority task is launched for the same interval.

Have you traced through the possibilities and determined this dummy-task approach to be OK?

Thanks for calling this out, let me evaluate this further and add some validations.
If it still doesn't seem viable, I will try an alternative.

(I suppose we could always remove that check from TaskLockbox, as the Task object fetched from storage is not really used for anything else).

locking changes removed from this PR.

gianm · 2026-08-06T07:33:20Z

+   */
+  public int markAllNonOvershadowedSegmentsAsUsed(String dataSource)
+  {
+    return markAsUsedWithExclusiveLock(


Given this requires an exclusive lock on eternity, will it be able to run on a datasource that is receiving active ingestion? I would think that it would require pausing ingestion? Seems like it would make it difficult to use.

Yes, that's true.

I suppose we could first identify the intervals that have eligible non-overshadowed segments that may be marked as used, and only try to lock those intervals.

locking changes removed from this PR.

gianm · 2026-08-06T07:36:06Z

+  public TaskLockType determineLockType()
  {
-    TaskLockType actualLockType;
+    final boolean useConcurrentLocks = Boolean.TRUE.equals(


This should coerce in the manner of QueryContexts#getAsBoolean, so "true" (the string) is treated as true too.

fixed, using the new utility method in TaskLocks.

gianm · 2026-08-06T07:37:13Z

+      return TaskLockType.REPLACE;
    } else {
-      actualLockType = getContextValue(Tasks.TASK_LOCK_TYPE, TaskLockType.EXCLUSIVE);
+      return getContextValue(Tasks.TASK_LOCK_TYPE, TaskLockType.EXCLUSIVE);


This should coerce too. As written I believe it will throw CastClassException on real JSON, since it'll try to cast the String values to TaskLockType.

…d_kill_2

kfaraz · 2026-08-06T09:44:25Z

Thanks for the feedback, @gianm . I have removed the locking changes from the PR for now.

  @Inject
  public UnusedSegmentsKiller(
      SegmentsMetadataManagerConfig config,
+      DefaultTaskConfig defaultTaskConfig,


FrankChen021

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

Reviewed 15 of 15 changed files. The dummy lock-probe and malformed-interval findings are resolved; capped-scan starvation remains in its existing thread, the shared-load-spec limit remains, and the update adds an explicit-lock-type regression.

This is an automated review by Codex GPT-5.6-Sol

FrankChen021 · 2026-08-06T12:36:47Z

-          null,
-          null,
+          MAX_SEGMENTS_TO_KILL_IN_BATCH,
+          candidate.numSegmentsToKill(),


[P1] Count metadata deletions against the candidate limit

candidate.numSegmentsToKill() becomes this task's limit, but KillUnusedSegmentsTask reduces the remaining limit only by segments deleted from deep storage even though SegmentNukeAction removes the entire fetched batch from metadata. For a 1,001-row candidate whose load specs are shared, the first batch can nuke 1,000 metadata rows while counting zero progress, and the second batch can nuke another 1,000. The duty can therefore exceed maxSegmentsToKill; track metadata rows processed for this limit or cap the embedded fetches independently.

FrankChen021 · 2026-08-06T12:36:47Z

    );

-    TaskLockType actualLockType = determineLockType(useConcurrentLocks);
+    final TaskLockType actualLockType = useConcurrentLocks ? TaskLockType.REPLACE : TaskLockType.EXCLUSIVE;


[P1] Preserve the explicit task lock type

This reduces every non-REPLACE context to EXCLUSIVE. For example, taskLockType: APPEND is parsed successfully by shouldUseConcurrentLocksForReplace but returns false here, so the task requests EXCLUSIVE instead of the explicitly requested APPEND lock. A programmatic TaskLockType.APPEND value also throws because QueryContexts.getAsEnum does not accept enum objects. This deterministically breaks KillUnusedSegmentsTaskTest#testIsReadyWithContextAppendLock; retain the parsed lock type when useConcurrentLocks does not select REPLACE.

gianm

This PR, now limited to the perf improvement, looks good to me. I believe the safety part would be split out into a separate PR, so consider removing the "make concurrent kill safer" text from the PR title. When the other PR is available please link it here too.

kfaraz · 2026-08-09T15:35:59Z

Thanks for the suggestions, @gianm !
As advised, I have created a new PR #19921 which adds a new KILL lock type for embedded kill tasks.

Improve efficiency of embedded kill tasks

61440d2

github-actions Bot added the Area - Ingestion label Jul 27, 2026

minor fixes

348837e

cryptoe approved these changes Jul 27, 2026

View reviewed changes

gianm reviewed Jul 27, 2026

View reviewed changes

FrankChen021 reviewed Jul 28, 2026

View reviewed changes

kfaraz added 2 commits July 28, 2026 17:43

Add config for maxSegmentsToKill

ba12061

skip unparseable rows

e9e1679

kfaraz requested a review from gianm July 28, 2026 16:13

kfaraz added 2 commits July 29, 2026 19:19

Use concurrent locks by default

d9e0749

Update docs

620dc68

github-actions Bot added the Area - Documentation label Jul 29, 2026

kfaraz requested a review from cryptoe July 29, 2026 14:44

Update javadoc

28a2c68

cryptoe reviewed Jul 29, 2026

View reviewed changes

cryptoe approved these changes Jul 29, 2026

View reviewed changes

Minor doc updates

d1d7368

FrankChen021 reviewed Jul 30, 2026

View reviewed changes

gianm reviewed Jul 30, 2026

View reviewed changes

kfaraz added 3 commits July 30, 2026 23:49

Acquire EXCLUSIVE lock while marking segments as used

e21bac2

Update docs

0f537bc

Fix test

1bf478e

kfaraz requested a review from gianm July 30, 2026 18:50

kfaraz changed the title ~~perf: Improve efficiency of embedded kill tasks~~ perf: Improve efficiency of embedded kill tasks and make concurrent kill safer Jul 31, 2026

Fly-Style self-requested a review July 31, 2026 08:48

FrankChen021 reviewed Jul 31, 2026

View reviewed changes

gianm reviewed Aug 6, 2026

View reviewed changes

kfaraz added 2 commits August 6, 2026 13:52

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

add76e1

…d_kill_2

Revert extra changes

7a3aee6

kfaraz added 3 commits August 6, 2026 15:06

cleanup

1b5c775

Fix binding

ca4208a

Remove timeout from test

c784d32

kfaraz requested a review from FrankChen021 August 6, 2026 09:47

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

View reviewed changes

Comment thread ...xing-service/src/main/java/org/apache/druid/indexing/overlord/duty/UnusedSegmentsKiller.java

@Inject

public UnusedSegmentsKiller(

SegmentsMetadataManagerConfig config,

DefaultTaskConfig defaultTaskConfig,

FrankChen021 reviewed Aug 6, 2026

View reviewed changes

Fix based on feedback

590711f

gianm approved these changes Aug 6, 2026

View reviewed changes

kfaraz changed the title ~~perf: Improve efficiency of embedded kill tasks and make concurrent kill safer~~ perf: Improve efficiency of embedded kill tasks Aug 7, 2026

kfaraz merged commit be27640 into apache:master Aug 7, 2026
44 of 46 checks passed

kfaraz deleted the improve_embedded_kill_2 branch August 7, 2026 04:05

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

kfaraz mentioned this pull request Aug 7, 2026

feat: Add new TaskLockType KILL for embedded kill tasks #19921

Merged

10 tasks

Uh oh!

Conversation

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

Uh oh!

Description

Changes

Uh oh!

cryptoe left a comment

Choose a reason for hiding this comment

Uh oh!

cryptoe Jul 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FrankChen021 left a comment

Choose a reason for hiding this comment

Uh oh!

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!

cryptoe 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!

kfaraz commented Jul 30, 2026

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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

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!

gianm 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!

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

kfaraz commented Jul 27, 2026 •

edited

Loading

cryptoe Jul 27, 2026 •

edited

Loading

kfaraz Jul 28, 2026 •

edited

Loading

kfaraz Jul 30, 2026 •

edited

Loading

kfaraz Aug 6, 2026 •

edited

Loading

kfaraz Aug 6, 2026 •

edited

Loading