[GitHub] jihoonson commented on a change in pull request #6086: Fix IllegalArgumentException in TaskLockBox.syncFromStorage() when updating from 0.12.x to 0.12.2

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

jihoonson commented on a change in pull request #6086: Fix IllegalArgumentException in TaskLockBox.syncFromStorage() when updating from 0.12.x to 0.12.2
URL: https://github.com/apache/incubator-druid/pull/6086#discussion_r206973678
 
 

 ##########
 File path: indexing-service/src/main/java/io/druid/indexing/overlord/TaskLockbox.java
 ##########
 @@ -382,12 +384,27 @@ private TaskLockPosse createOrFindLockPosse(Task task, TaskLock taskLock)
           taskLock.getDataSource(),
           task.getDataSource()
       );
-      Preconditions.checkArgument(
-          task.getPriority() == taskLock.getPriority(),
-          "lock priority[%s] is different from task priority[%s]",
-          taskLock.getPriority(),
-          task.getPriority()
-      );
+
+      final int priority;
+      // Here, both task and taskLock can return 0 priority if the priority is not properly set, that is they are
+      // created in an older version of Druid.
+      if (task.getPriority() == taskLock.getPriority()) {
+        priority = task.getPriority();
+      } else {
+        // The priority of task and taskLock can be different when updating cluster if the task doesn't have the
+        // priority in taskContext while taskLock has. In this case, we simply ignores the task priority and uses the
+        // taskLock priority.
+        if (task.getContextValue(Tasks.PRIORITY_KEY) == null && task.getDefaultPriority() == taskLock.getPriority()) {
 
 Review comment:
   @gianm here is the thing.
   
   Before 0.12, both task and taskLock don't have priority. And before #6050, the task works as you described above while the taskLock returns 0 if priority doesn't exist. So, when `taskLockBox.syncFromStorage()` is called, they work like below.
   
   priority container | value
   ------------ | -------------
   task | no priority
   taskLock | no priority
   task.getPriority() | default priority depending on task type
   taskLock.getPriority() | 0
   
   This causes the priority mismatch between task and taskLock, so https://github.com/apache/incubator-druid/pull/6050 was to fix this issue. In that PR, I've changed to inject the default priority to submitted tasks if they don't have priority. So, now `task.getPriority()` returns the priority what it has or 0 which is the same with how `taskLock.getPriority()` works.
   
   However, after this patch, `taskLockBox.syncFromStorage()` can introduce the below situation.
   
   priority container | value
   ------------ | -------------
   task | no priority
   taskLock | default priority depending on task type
   task.getPriority() | 0
   taskLock.getPriority() | default priority depending on task type
   
   This can happen because the priority of taskLock is initialized with `task.getPriority()` which returns the default priority if priority doesn't exist in taskContext before https://github.com/apache/incubator-druid/pull/6050. So, if the task was created before https://github.com/apache/incubator-druid/pull/6050, the priority of task and its taskLock would be different.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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


With regards,
Apache Git Services

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