[GitHub] [druid] jihoonson commented on a change in pull request #12041: Allow for appending tasks to co-exist with each other.

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

jihoonson commented on a change in pull request #12041:
URL: https://github.com/apache/druid/pull/12041#discussion_r765278116



##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/overlord/TaskLockbox.java
##########
@@ -1136,6 +1136,9 @@ boolean reusableFor(LockRequest request)
           case SHARED:
             // All shared lock is not reusable. Instead, a new lock posse is created for each lock request.

Review comment:
       Can you fix this comment? It seems no longer true.

##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/overlord/TaskLockbox.java
##########
@@ -581,7 +581,7 @@ private boolean isTaskLocksValid(Task task, List<Interval> intervals)
             final List<TaskLockPosse> lockPosses = getOnlyTaskLockPosseContainingInterval(task, interval);
             // Tasks cannot enter the critical section with a shared lock

Review comment:
       Can you remove this comment?

##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/overlord/TaskLockbox.java
##########
@@ -581,7 +581,7 @@ private boolean isTaskLocksValid(Task task, List<Interval> intervals)
             final List<TaskLockPosse> lockPosses = getOnlyTaskLockPosseContainingInterval(task, interval);
             // Tasks cannot enter the critical section with a shared lock
             return lockPosses.stream().map(TaskLockPosse::getTaskLock).allMatch(

Review comment:
       > [ERROR] indexing-service/src/main/java/org/apache/druid/indexing/overlord/TaskLockbox.java:583 -- Stream.allMatch(x -> !(...)) can be replaced with noneMatch(...)
   
   Intellij inspection failed at this line.

##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/common/task/AbstractBatchIndexTask.java
##########
@@ -286,10 +286,11 @@ public boolean determineLockGranularityAndTryLock(TaskActionClient client, List<
         Tasks.FORCE_TIME_CHUNK_LOCK_KEY,
         Tasks.DEFAULT_FORCE_TIME_CHUNK_LOCK
     );
+    final boolean useSharedLock = getContextValue(Tasks.USE_SHARED_LOCK, false);

Review comment:
       Should we also check if `appendingToExisting` is set to true?




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