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_r206804681
##########
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 looked into it a bit more and I think the issue is that Task.java has:
```
default int getPriority()
{
return getContextValue(Tasks.PRIORITY_KEY, Tasks.DEFAULT_TASK_PRIORITY);
}
```
And NoopTask does not override this. Since `Tasks.DEFAULT_TASK_PRIORITY` is
0, NoopTask has 0 priority, and so the tests are too easy to pass.
I think Task should change to:
```
default int getPriority()
{
return getContextValue(Tasks.PRIORITY_KEY, getDefaultPriority());
}
```
That way, the behavior of `getPriority()` is consistent with
`getDefaultPriority()`. It seems like the only reason that priorities work at
all today is that the OverlordResource injects them with this code:
```
// Set default priority if needed
final Integer priority =
task.getContextValue(Tasks.PRIORITY_KEY);
if (priority == null) {
task.addToContext(Tasks.PRIORITY_KEY,
task.getDefaultPriority());
}
```
I think this won't be necessary if `Task.getPriority()` is fixed.
----------------------------------------------------------------
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:
[email protected]
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
