[GitHub] [incubator-druid] leventov commented on issue #8809: Prohibit Futures.addCallback(Future, Callback)

[GitBox <gi...@apache.org>]

leventov commented on issue #8809: Prohibit Futures.addCallback(Future, Callback)
URL: https://github.com/apache/incubator-druid/pull/8809#issuecomment-549022248
 
 
   Mechanically adding `Execs.directExecutor()` doesn't solve the problem. You should actually thoughtfully check every case and verify that `Execs.directExecutor()` is an acceptable option in each of them. A case should meet at least one of three criteria, as described in https://github.com/code-review-checklists/java-concurrency#cf-beware-non-async:
    1) The callback is lightweight and non-blocking (as is the case in your PR #8700); or
    2) The callback is added from the same executor as the future is completed; or
    3) The callback attachment is preceded with `if (future.isDone())` check.
   
   Then, the specific criterion should be identified in a comment, similarly to what I asked you to do here: https://github.com/apache/incubator-druid/pull/8700#discussion_r341482427.
   
   Otherwise (if the case meets neither of these criteria), `directExecutor()` should not be used.

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org