Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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-17439: Make polling for new records an explicit action/event in the new consumer #17035
KAFKA-17439: Make polling for new records an explicit action/event in the new consumer #17035
Changes from 1 commit
e984638
b6af23b
335c249
2abd6a4
45bc8c0
7265404
5449549
f7a5940
7acf732
7cda968
de2c961
a0beb14
af96038
09e951e
fbd147c
4c7c1bf
3aed5e7
b097ebe
62ccaa6
01b5d7a
aae7e97
b942a46
63abc91
a98e8b0
d6e2241
1e832e0
4071849
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
at this point we may already have records in hand to return (consumed position updated), so we should be very careful to not throw any error here. But this
sendFetches
could throw interrupted because of theaddAndGet
right?Shouldn't we just do a best effort to pipeline the next requests using
add
instead ofaddAndGet
? It would achieve what we want, removing the risk of errors, and it would actually align better with what the classic does on thissendFetches
+transmitSends
:kafka/clients/src/main/java/org/apache/kafka/clients/consumer/internals/ConsumerNetworkClient.java
Lines 327 to 329 in 140d35c
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.
Done.
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.
The actual poll now happens in here (addAndGet that will complete when the background has had one run, called fetchMgr.poll), so should the log line on ln 1538 "Polling for fetches with timeout..." be right before this?
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.
We're not polling for the incoming responses in
sendFetches()
, just enqueuing the outgoing requests. This mimics theClassicKafkaConsumer
in that the requests are enqueued in itssendFetches()
but then toward the bottom ofpollForFetches()
client.poll()
is invoked to wait for the results of the fetch requests.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.
well the
sendFetches
blocks until theCreateFetchRequestsEvent
completes, and that only happens on fetchMgr.pollkafka/clients/src/main/java/org/apache/kafka/clients/consumer/internals/FetchRequestManager.java
Line 115 in aae7e97
So when the
sendFetches
completes we did poll the manager right? (and depending on time, maybe we did poll the client.poll too, which happens in the background right after polling all managers). That's why the log for "Polling for fetches" made sense to me before thesendFetches
, but am I missing another poll happening after the log line maybe? (where it is now)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.
The two
ConsumerDelegate
implementations work differently:AsyncKafkaConsumer
:FetchRequestManager.poll()
will complete the event'sFuture
on the background thread before it exits, i.e. before the thread starts the network I/O. Completing theFuture
starts the application thread racing toward logging that message and the background thread racing toward starting network I/O. I'll admit—I haven't dug through the code to surmise the relative costs of each thread's work before either cross their respective finish lines to win.ClassicKafkaConsumer
:Fetcher.sendFetchesInternal()
callsConsumerNetworkClient.send()
to enqueue the request, but then it callsNetworkClient.wakeup()
. Since the sameConsumerNetworkClient
instance used by the consumer is also used byAbstractCoordinator.HeartbeatThread
, it's technically possible that the heartbeat thread'srun()
method could start network I/O when it callsNetworkClient.pollNoWakeup()
. Granted, that's a race that the application thread is much more likely to win given that the heartbeat thread runs much less frequently.Here are some points to consider:
TRACE
, which I assume is very rare for users to enable.Given the above, I'm of the opinion that it's an exercise in hair splitting to alter the logging. However, I could also just change it which would have been way less effort than researching, thinking, and composing this response 🤣
If we leave the log line as it is, what would the effect be for the user?
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.
I surely didn't intend for you to put up that long response he he, sorry. It's not about the log line per-se, it's about the alignment on where the poll happens. The classic consumer logs "Polling for records", then calls
client.poll
. vs us here we dosendFetches
(which triggers the client.poll async in the background thread because it blocks until we poll the fetch manager), then log "Polling for fetches...".That's the diff I saw and just wanted to understand/align on where the poll happens: once we trigger
sendFetches
(blocking), the client.poll will happen in the background anytime, not controlled by the app thread. Agreed? If so I'm ok with leaving the log unchanged, understanding it could come out after the client.poll happened.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.
Agreed—the background thread is going to move from calling each of the
RequestManager
’spoll()
method toNetworkClient.poll()
method without the intervention of the application thread.Thanks!