FrankChen021 commented on code in PR #19772:
URL: https://github.com/apache/druid/pull/19772#discussion_r3728799359
##########
indexing-service/src/main/java/org/apache/druid/indexing/overlord/duty/UnusedSegmentsKiller.java:
##########
@@ -436,33 +471,24 @@ private EmbeddedKillTask(
{
super(
taskId,
- candidate.dataSource,
- candidate.interval,
+ candidate.dataSource(),
+ candidate.interval(),
null,
Map.of(Tasks.PRIORITY_KEY,
Tasks.DEFAULT_EMBEDDED_KILL_TASK_PRIORITY),
- null,
- null,
+ MAX_SEGMENTS_TO_KILL_IN_BATCH,
+ candidate.numSegmentsToKill(),
Review Comment:
[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.
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/task/KillUnusedSegmentsTask.java:
##########
@@ -511,14 +517,12 @@ public BroadcastDatasourceLoadingSpec
getBroadcastDatasourceLoadingSpec()
@Override
public boolean isReady(TaskActionClient taskActionClient) throws Exception
{
- final boolean useConcurrentLocks = Boolean.TRUE.equals(
- getContextValue(
- Tasks.USE_CONCURRENT_LOCKS,
- Tasks.DEFAULT_USE_CONCURRENT_LOCKS
- )
+ final boolean useConcurrentLocks =
TaskLocks.shouldUseConcurrentLocksForReplace(
+ getContext(),
+ Tasks.DEFAULT_USE_CONCURRENT_LOCKS
);
- TaskLockType actualLockType = determineLockType(useConcurrentLocks);
+ final TaskLockType actualLockType = useConcurrentLocks ?
TaskLockType.REPLACE : TaskLockType.EXCLUSIVE;
Review Comment:
[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`.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
