KAFKA-20915: Fix for fetch buffer wake ups in AsyncConsumer - #23124
KAFKA-20915: Fix for fetch buffer wake ups in AsyncConsumer#23124lianetm wants to merge 2 commits into
Conversation
| // "Wake" the fetch buffer on any response, even if it's empty, to allow the consumer to not block | ||
| // indefinitely waiting on the fetch buffer to get data. | ||
| if (needsWakeup) | ||
| fetchBuffer.wakeup(); |
There was a problem hiding this comment.
this wake up moves to the removePendingFetchRequest to ensure it's triggered from the failed fetch path too
| // needlessly wait for a wakeup that won't come: it can still collect data that a node may have | ||
| // buffered, and submit a new poll event to generate the next request. | ||
| // Only do so, though, when no response is still expected and a fetch request could actually be generated. | ||
| if (nodesWithPendingFetchRequests.isEmpty() && hasFetchablePartitions()) |
There was a problem hiding this comment.
this condition is the fix for the main issue
|
Is this duplicate to #23014? |
|
Oh yes @chia7712 , same underlying issue indeed (even though we landed on that coming from different angles). I will close this one then, and take a closer look at yours. Thanks! |
|
Hello @lianetm, I think your approach is better and cleaner than mine, so I'm happy to close my patch. |
|
No worries, I had closed this already, and actually the approach on the other PR is better imo, covers more, because it's getting the info from the prepare func (the one who knows why exactly we are generating no requests). With that we can cover not only the inflight and non fetchable case I was covering here, but also the backoff, leader not available etc, nice. Will take a closer look at your PR. Thanks! |
3 participants
The async consumer wakes up the buffer when adding data to it (all good,
unchanged with this PR, unblocks the app thread to collect the data).
But it also wakes the buffer when it attempts to generate fetch requests
and it can't generate any. This included the case when fetch request was
already in-flight (so can't generate any more fetch requests). This
leads to a spin between the 2 threads:
wakes the buffer, completes the event
submits another poll event
No blocking and no backoff, so spinning generating and completing poll
events for as long as a request/response is inflight.
This PR includes a fix to ensure the explicit wakeup triggered when no
requests can be generated fires only when it can achieve something
(nothing is in flight, and some partition is fetchable).
It also includes a fix to ensure that every in-flight request wakes the
buffer when it completes, no matter the outcome. This ensures progress
in all cases and aligns with the classic consumer (it blocks on the
network poll so effectively unblocks on any fetch outcome)
Reviewers: Chia-Ping Tsai chia7712@gmail.com, Ken Huang s7133700@gmail.com