Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

KAFKA-12156: Document single threaded response handling in Admin client #9842

Merged
merged 1 commit into from Jan 11, 2021

Conversation

tombentley
Copy link
Contributor

As described in KAFKA-12156, if users block the response handling thread in one call waiting for the result of a second "nested" call then the client effectively hangs because the 2nd call's response will never be processed.

If users block the response handling thread in one call waiting for the result
of a second "nested" call then the client effectively hangs because the 2nd
call's response will never be processed.
@tombentley
Copy link
Contributor Author

@chia7712 please could you review?

@chia7712
Copy link
Contributor

chia7712 commented Jan 7, 2021

This is similar to #9707

The document is good but throwing a exception to break deadlock is more meaningful for me. Users can read the error message straightforward. WDYT?

@tombentley
Copy link
Contributor Author

@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.

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?

@chia7712
Copy link
Contributor

chia7712 commented Jan 8, 2021

@tombentley Thanks for your feedback!

Or did you have some other idea about how this could be done?

no, I don't have idea for simple/compatible solution :(

I will take a look at the JavaDocs later.

Copy link
Contributor

@chia7712 chia7712 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tombentley Thanks for improved JavaDocs. LGTM

@chia7712 chia7712 merged commit 39a9274 into apache:trunk Jan 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants