kirktrue commented on PR #20324: URL: https://github.com/apache/kafka/pull/20324#issuecomment-3250946011
> Thanks for the updates! I'm afraid we are still introducing gaps (comment below) by splitting the logic of the existing `CheckAndUpdatePositionsEvent` event into the app thread and background thread, and honestly I don't see how we could sort them out cleanly and confidently with this split approach. But let's not surrender :) > > Actually not only for this PR but also the one attempting a similar approach for the `PollEvent`, I wonder if instead of splitting current events logic into the app and background thread, we should explore consolidating them in the background, closer to the fetch logic. This is my reasoning: > > If we take a step back, both `CheckAndUpdatePositionsEvent` and `PollEvent` do stuff that only matters for fetching (we use them in the app thread, in a blocking way, only to ensure things happen before the actual fetching, which in the end runs in the background too). > > This is what we currently have (before this PR): > > * `PollEvent` to trigger reconciliation and auto-commit -> must happen before updating positions > * `CheckAndUpdatePositionsEvent` to throw previous errors and update positions -> must happen before fetching > * `CreateFetchRequestsEvent` -> triggers the actual fetching > > So what about we move it all to the fetch in the background, and when processing a `CreateFetchRequestEvent` we: > > 1. trigger reconciliation & auto-commit (what the PollEvent used to do) > 2. update fetch positions (what the update fetch positions used to do) > 3. trigger fetch > > Note we still need to throw previous exceptions related to the `updateFetchPositions` before returning data from the buffer, but that part can be safely done in the app thread using AtomicRef at the beginning of the poll. There could be other things to consider, just high level ideas for now in case it helps. Thoughts? Thanks for the explanation, @lianetm! I definitely like the idea of having a consolidated `DoEverythingNeededForFetchEvent` that handles the logic that's currently split up across `PollEvent`, `CheckAndUpdatePositionsEvent`, and `CreateFetchRequestsEvent`. Unfortunately, I see two constraints that prevent the ability to consolidate those events: 1. Offset commit callbacks, background events, and collecting fetch data must be executed in the application thread 2. The execution of those callbacks and events in the application thread are interleaved with the event logic executed in the background thread for `PollEvent`, `CheckAndUpdatePositionsEvent`, and `CreateFetchRequestsEvent` If there's a way around these two issues, there's hope. I just don't yet see a way around those constraints without a major change 😞 -- 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
