kirktrue commented on code in PR #15640:
URL: https://github.com/apache/kafka/pull/15640#discussion_r1586557142
##########
clients/src/main/java/org/apache/kafka/clients/consumer/internals/AsyncKafkaConsumer.java:
##########
@@ -1892,13 +1897,13 @@ private void subscribeInternal(Collection<String>
topics, Optional<ConsumerRebal
* @return {@code true} if the event completed within the timeout, {@code
false} otherwise
*/
// Visible for testing
- <T> T processBackgroundEvents(EventProcessor<?> eventProcessor,
+ <T> T processBackgroundEvents(EventProcessor<BackgroundEvent>
eventProcessor,
Review Comment:
I made the documentation changes you requested and removed the logging to
make the logic simpler.
When you state that it seems like "overkill to have all this code for
something we don't need now, or know if we we'll need some day," I'm a bit
confused 🤔 Because unsubscribing may require invoking
`ConsumerRebalanceListener` callbacks, we need a way to check and run those
events that are coming from the background thread, right.
I do agree that it's overkill to have this broken out as a separate method
since it's only used for the `unsubscribe()` case. IIRC, there was some talk of
another use case for this, and it does make unit testing it easier.
##########
clients/src/main/java/org/apache/kafka/clients/consumer/internals/AsyncKafkaConsumer.java:
##########
@@ -1892,13 +1897,13 @@ private void subscribeInternal(Collection<String>
topics, Optional<ConsumerRebal
* @return {@code true} if the event completed within the timeout, {@code
false} otherwise
*/
// Visible for testing
- <T> T processBackgroundEvents(EventProcessor<?> eventProcessor,
+ <T> T processBackgroundEvents(EventProcessor<BackgroundEvent>
eventProcessor,
Review Comment:
I made the documentation changes you requested and removed the logging to
make the logic simpler.
When you state that it seems like "overkill to have all this code for
something we don't need now, or know if we we'll need some day," I'm a bit
confused 🤔 Because unsubscribing may require invoking
`ConsumerRebalanceListener` callbacks, we need a way to check and run those
events that are coming from the background thread, right?
I do agree that it's overkill to have this broken out as a separate method
since it's only used for the `unsubscribe()` case. IIRC, there was some talk of
another use case for this, and it does make unit testing it easier.
--
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.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
