leventov commented on a change in pull request #8809: Prohibit
Futures.addCallback(Future, Callback)
URL: https://github.com/apache/incubator-druid/pull/8809#discussion_r342945408
##########
File path:
indexing-service/src/main/java/org/apache/druid/indexing/overlord/TaskQueue.java
##########
@@ -556,7 +559,8 @@ private void handleStatus(final TaskStatus status)
.emit();
}
}
- }
+ },
+ statusHandler
Review comment:
It's non-obvious from this point in code to see that `attachCallbacks()` is
called from a different thread pool than `statusFuture` is completed.
There are options to improve:
- Propagate `Executor` argument and pass `statusHandler` only in
`manage()`, and, additionally, amend the javadoc comment for `manage()` noting
that it is run from `{@link #managerExec}`.
- Just add a more elaborate comment right here.
In either case, it will be non-obvious for readers why different executors
demand a special executor. So the comment should look something like that:
"Using dedicated statusHandler executor instead of directExecutor() because
the callback's onSuccess() is not trivial and because the statusFuture is
completed in some incapsulated thread pool in TaskRunner; directExecutor() may
create operational instability and subtle dependency between components here.
See
https://github.com/code-review-checklists/java-concurrency#cf-beware-non-async
for details."
Something similar should be done in all other places where you use a custom
executor instead of directExecutor().
----------------------------------------------------------------
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.
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]
