tombentley commented on pull request #9842:
URL: https://github.com/apache/kafka/pull/9842#issuecomment-756637958


   @chia7712 thanks for taking a look. I agree that documenting it isn't a 
great solution.
   
   Generally speaking, it's an anti-pattern for the user to go running blocking 
code in the future completion handler unless guarantees are provided about the 
thread on which that code will run (which we can't assume here, since it's 
never been documented). It's not just the blocking methods on KafkaFuture which 
are problematic, any blocking method would be enough, e.g. 
   
   ```java
   admin.listTopics()...thenApply(topics ->  {
     Thread.sleep(60_000);
     // blocks the AdminClientRunnable preventing timely handling of other 
responses
   })
   ```
   
   I don't think running these completion handlers concurrently is a solution 
either. It would introduce thread safety issues if completion handlers started 
to be executed concurrently, since it could introduce a need for coordination 
between handlers which never existed before (even if though the single threaded 
behaviour wasn't guaranteed). 
   
   So I think the only solution is to log a warning if the execution of a 
future completion takes longer that some expected upper bound. (Throwing an 
exception isn't possible in the general case, e.g. the `sleep` example above). 
We'd need a separate thread to do this, and in the normal case (where there was 
no blocking in the handler) this would be effectively doing nothing. I'm happy 
to do it, if people think that this is a sufficiently nasty problem to warrant 
a thread just for some warn logging.
   
   Or did you have some other idea about how this could be done?


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


Reply via email to