jtuglu-netflix commented on code in PR #18020:
URL: https://github.com/apache/druid/pull/18020#discussion_r2097161014
##########
indexing-service/src/main/java/org/apache/druid/indexing/overlord/TaskQueue.java:
##########
@@ -531,6 +531,8 @@ public boolean add(final Task task)
// insert the task into our queue. So don't catch it.
final DateTime insertTime = DateTimes.nowUtc();
taskStorage.insert(task, TaskStatus.running(task.getId()));
+ // Note: the TaskEntry created for this task doesn't actually use the
`insertTime` timestamp, it uses a new
+ // timestamp created in the ctor. This prevents races from occurring
while syncFromStorage() is happening.
Review Comment:
> It would make the behaviour of the TaskEntry.lastUpdatedTime more
consistent.
Could you give an example where it would be more consistent? Using a
timestamp T that could be strictly smaller than the actual time the value was
added to `activeTasks` seems to me like it could potentially cause more harm
than good. I feel like we should probably keep the timestamp as close as
possible to the time the value was actually updated in the map.
What I mean by that is there could be cases where there are concurrent
accesses to the map where those timestamp values' orderings don't dictate the
actual ordering of when the map values were updated (even though these clock
reads are all from the same local clock).
--
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]
