lianetm commented on code in PR #18737:
URL: https://github.com/apache/kafka/pull/18737#discussion_r1954732895
##########
clients/src/main/java/org/apache/kafka/clients/consumer/internals/AbstractMembershipManager.java:
##########
@@ -818,6 +826,8 @@ void maybeReconcile() {
return;
}
+ if (autoCommitEnabled && !canCommit) return;
Review Comment:
> we could apply the same approach we use for auto-commit and auto-commit on
revocation. (offsetsReady)
Could it be simpler here? The commit event has a future that only completes
when the request gets a response, so we couldn't wait for that on the app
thread. But the `PollEvent` is very different, it doesn't even have a future at
the moment (it only performs actions locally). So wouldn't it be enough to make
the `PollEvent` completable and wait for it (meaning it would wait for local
actions that need to happen on the background, no network-io at all).
This is a key point btw, we're not performing any network IO when processing
a `PollEvent`, even if we wait for it's completion in the app thread, because
the `PollEvent` only generates async commits. No other request involved, never
waits for a response. I could be missing something, but seems to me that we
wouldn't be breaking the separation of responsabilities (background thread is
still the one in charge of the network IO in this case, because the network IO
will happen when we poll the `commitRequestManager`, that will send the async
commit that the `PollEvent` generated. The app thread just waited for the
generation, to ensure that it could move into the fetching phase)
All that being said, the fetching happening in the app thread is probably
the elephant in the room here, but that is definitely food for thought for
after 4.0 :)
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
