[GitHub] [druid] kfaraz commented on a diff in pull request #13390: Fix shared lock acquisition criteria

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

kfaraz commented on code in PR #13390:
URL: https://github.com/apache/druid/pull/13390#discussion_r1027337562


##########
indexing-service/src/main/java/org/apache/druid/indexing/overlord/TaskLockbox.java:
##########
@@ -474,8 +474,12 @@ private TaskLockPosse createOrFindLockPosse(LockRequest request)
         if (reusablePosses.size() == 0) {
           // case 1) this task doesn't have any lock, but others do
 
-          if (request.getType().equals(TaskLockType.SHARED) && isAllSharedLocks(conflictPosses)) {
-            // Any number of shared locks can be acquired for the same dataSource and interval.
+          if (request.getType().equals(TaskLockType.SHARED)
+              && areAllEqualOrHigherPriorityLocksSharedOrRevoked(conflictPosses, request.getPriority())) {
+            // Any number of shared locks can be acquired for the same dataSource and interval
+            // Non-shared locks of equal or greater priority, if present, must already be revoked
+            // Other exclusive (or any non-shared) locks of lower priority can be revoked

Review Comment:
   Shared and exclusive locks are the only two types of locks.
   So the phrase `exclusive (or any non-shared) locks` can cause confusion to readers. 
   If we add a new type of lock in the future, this can be updated.



##########
indexing-service/src/main/java/org/apache/druid/indexing/overlord/TaskLockbox.java:
##########
@@ -1070,10 +1074,33 @@ Map<String, NavigableMap<DateTime, SortedMap<Interval, List<TaskLockPosse>>>> ge
     return running;
   }
 
-  private static boolean isAllSharedLocks(List<TaskLockPosse> lockPosses)
+  /**
+   * Check if all lockPosses are either shared
+   * OR of lower priority
+   * OR are revoked non-shared locks if their priorities are greater than or equal to the provided priority
+   * @param lockPosses conflicting task lock posses to be checked
+   * @param priority priority of the lock to be acquired
+   * @return true if the condititons are met
+   */
+  private static boolean areAllEqualOrHigherPriorityLocksSharedOrRevoked(List<TaskLockPosse> lockPosses, int priority)
   {
     return lockPosses.stream()
-                     .allMatch(taskLockPosse -> taskLockPosse.getTaskLock().getType().equals(TaskLockType.SHARED));
+                     .filter(taskLockPosse -> taskLockPosse.getTaskLock().getPriority() >= priority)

Review Comment:
   Better use the non-null priority as done in `isRevocable`.



-- 
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: commits-unsubscribe@druid.apache.org

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