maytasm commented on a change in pull request #11190:
URL: https://github.com/apache/druid/pull/11190#discussion_r645129523
##########
File path:
indexing-service/src/main/java/org/apache/druid/indexing/overlord/TaskStorageQueryAdapter.java
##########
@@ -62,9 +62,9 @@ public TaskStorageQueryAdapter(TaskStorage storage,
TaskLockbox taskLockbox)
*
* @return Map from Task Id to locked intervals.
Review comment:
Is this Map from datasource to locked intervals?
##########
File path:
server/src/main/java/org/apache/druid/server/coordinator/duty/CompactSegments.java
##########
@@ -115,11 +114,17 @@ public DruidCoordinatorRuntimeParams
run(DruidCoordinatorRuntimeParams params)
.stream()
.collect(Collectors.toMap(DataSourceCompactionConfig::getDataSource,
Function.identity()));
final List<TaskStatusPlus> compactionTasks =
filterNonCompactionTasks(indexingServiceClient.getActiveTasks());
- // dataSource -> list of intervals of compaction tasks
- final Map<String, List<Interval>> compactionTaskIntervals =
Maps.newHashMapWithExpectedSize(
+ // dataSource -> list of intervals for which compaction will be
skipped in this run
+ final Map<String, List<Interval>> intervalsToSkipCompaction =
Maps.newHashMapWithExpectedSize(
compactionConfigList.size());
- final Map<String, DatasourceIntervals> taskToLockedIntervals = new
HashMap<>(
- indexingServiceClient.getLockedIntervals());
+
+ // Skip all the intervals locked by higher priority tasks for each
datasource
+ getLockedIntervalsToSkip(compactionConfigList).forEach(
Review comment:
Is this needed? Doesn't getLockedIntervalsToSkip already return
dataSource -> list of intervals
##########
File path:
indexing-service/src/test/java/org/apache/druid/indexing/overlord/TaskLockboxTest.java
##########
@@ -1149,29 +1149,24 @@ public void testGetLockedIntervals()
);
// Verify the locked intervals
Review comment:
Can you add test where the existing taskLock priority is lower than the
minTaskPriority argument (and hence the lock should not be returned)?
##########
File path:
indexing-service/src/main/java/org/apache/druid/indexing/overlord/TaskLockbox.java
##########
@@ -676,61 +675,63 @@ public TaskLock apply(TaskLockPosse taskLockPosse)
}
/**
- * Gets a Map containing intervals locked by active tasks. Intervals locked
- * by revoked TaskLocks are not included in the returned Map.
+ * Gets a List of Intervals locked by higher priority tasks for each
datasource.
+ * Here, Segment Locks are being treated the same as Time Chunk Locks i.e.
+ * a Task with a Segment Lock is assumed to lock a whole Interval and not
just
+ * the corresponding Segment.
*
- * @return Map from Task Id to locked intervals.
+ * @param minTaskPriority Minimum task priority for each datasource. Only the
+ * Intervals that are locked by Tasks higher than this
+ * priority are returned. Tasks for datasources that
+ * are not present in this Map are not returned.
+ * @return Map from Datasource to List of Intervals locked by Tasks that have
+ * priority strictly greater than the {@code minTaskPriority} for that
datasource.
Review comment:
should this be > or >= the {@code minTaskPriority}?
Do we want to submit auto compaction task if there is a task with equal task
priority already running?
--
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
