[GitHub] [druid] maytasm commented on a change in pull request #11190: Temporarily skip compaction for locked intervals

[GitBox <gi...@apache.org>]

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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org