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 (approximately) 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 (in
this case it's guaranteed to be ≥ all other timestamps for the same task ID
since the clock read happens under lock).
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]
