jtuglu1 commented on code in PR #18448:
URL: https://github.com/apache/druid/pull/18448#discussion_r2309426975
##########
indexing-service/src/main/java/org/apache/druid/indexing/overlord/TaskQueue.java:
##########
@@ -548,27 +550,32 @@ public boolean add(final Task task)
}
@GuardedBy("startStopLock")
- private void addTaskInternal(final Task task, final DateTime updateTime)
+ private void addTaskInternal(final TaskInfo<Task, TaskStatus> taskInfo,
final DateTime updateTime)
{
final AtomicBoolean added = new AtomicBoolean(false);
final TaskEntry entry = addOrUpdateTaskEntry(
- task.getId(),
+ taskInfo.getId(),
prevEntry -> {
if (prevEntry == null) {
added.set(true);
- return new TaskEntry(task);
+ return new TaskEntry(taskInfo);
} else if (prevEntry.lastUpdatedTime.isBefore(updateTime)) {
prevEntry.lastUpdatedTime = updateTime;
}
+ // Ensure we keep the current status up-to-date
+ if (!prevEntry.taskInfo.getStatus().equals(taskInfo.getStatus())) {
+ prevEntry.taskInfo = taskInfo;
Review Comment:
> This update should happen inside the if clause before this, so that we
have a consistent view.
Yeah, this seems right to me – we don't want to assign an old/stale status.
I think pairing these 2 updates inside a `TaskEntry` helper makes sense, as
it'll ensure that future updates to the code always pair these updates
together. In this case, it's a bit lucky that `notifyStatus` is always wrapped
by the `updateTaskEntry`, which ensures the `lastUpdatedTime`.
##########
indexing-service/src/main/java/org/apache/druid/indexing/overlord/TaskQueue.java:
##########
@@ -548,27 +550,32 @@ public boolean add(final Task task)
}
@GuardedBy("startStopLock")
- private void addTaskInternal(final Task task, final DateTime updateTime)
+ private void addTaskInternal(final TaskInfo<Task, TaskStatus> taskInfo,
final DateTime updateTime)
{
final AtomicBoolean added = new AtomicBoolean(false);
final TaskEntry entry = addOrUpdateTaskEntry(
- task.getId(),
+ taskInfo.getId(),
prevEntry -> {
if (prevEntry == null) {
added.set(true);
- return new TaskEntry(task);
+ return new TaskEntry(taskInfo);
} else if (prevEntry.lastUpdatedTime.isBefore(updateTime)) {
prevEntry.lastUpdatedTime = updateTime;
}
+ // Ensure we keep the current status up-to-date
+ if (!prevEntry.taskInfo.getStatus().equals(taskInfo.getStatus())) {
+ prevEntry.taskInfo = taskInfo;
Review Comment:
> This update should happen inside the if clause before this, so that we
have a consistent view.
Yeah, this seems right to me – we don't want to assign an old/stale status.
I think pairing these 2 updates inside a `TaskEntry` helper makes sense, as
it'll ensure that future updates to the code always pair these updates
together. In this case, it's a bit lucky that `notifyStatus` is always wrapped
by the `updateTaskEntry`, which ensures the `lastUpdatedTime`.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
