[GitHub] gianm 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>]

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

 ##########
 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:
   I don't understand this line. If we have a task lock saved from an earlier version of Druid with missing priority (so it becomes 0 when read from the DB) and a task running with no priority specified, then:
   
   - `task.getContextValue(Tasks.PRIORITY_KEY)` is null
   - `task.getDefaultPriority()` is some default priority like 75
   - `taskLock.getPriority()` is 0
   
   So this expression is overall false, and we will go to the else block and throw an exception. It seems like the problem mentioned in the PR description is not fixed? But if the unit tests pass, how can that be?

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