-
Notifications
You must be signed in to change notification settings - Fork 14.6k
MINOR: extend consumer close java doc with error handling behaviour #20472
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lianetm thanks for this improvement !
@@ -1833,6 +1838,9 @@ public void close(Duration timeout) { | |||
* {@link ConsumerConfig#REQUEST_TIMEOUT_MS_CONFIG} for these requests to complete during the close operation. | |||
* Note that the execution time of callbacks (such as {@link OffsetCommitCallback} and | |||
* {@link ConsumerRebalanceListener}) does not consume time from the close timeout. | |||
* <p> | |||
* This close operation will attempt all shutdown steps even if one of them fails. | |||
* It logs all encountered errors, continues to execute the next steps, and finally throws the first error found. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also list the exceptions, as we do for other close methods?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, makes sense to me to align these comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR. For KafkaConsumer
, the changes look good subject to the comment already left. Please could you apply the same change to KafkaShareConsumer
.
Thank you all for the reviews. Comments addressed adding missing exceptions to the KafkaConsumer (and checked the ShareKafkaConsumer has them already consistently). |
@lianetm Checkstyle :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the doc improvement
the root cause of flaky is https://github.com/apache/kafka/pull/20244/files#r2325557949 |
Add to the consumer.close java doc to describe the error handling
behaviour.
Reviewers: Matthias J. Sax [email protected], Chia-Ping Tsai
[email protected], Andrew Schofield [email protected],
TengYao Chi [email protected]