lianetm commented on PR #18737: URL: https://github.com/apache/kafka/pull/18737#issuecomment-2622148387
Hey @frankvicky , thanks for tackling this promptly! High level comment, we've been intentionally keeping most of the subscription state actions into the background thread, to avoid race conditions that we had in the past (ex. app thread trying to access subscription state info for a partition that was no longer assigned because the background had removed it with a reconciliation). The the trick is that the subscription state changes in the background, ex. when reconciliation completes and updates assignment, when events like `seek` or `updateFetchPositions` are processed and update positions). So thinking about this case, what about the option of having the `PollEvent` processing (in the background thread) trigger all actions the `CommitMgr` has to at the beginning of each poll iteration: 1. update auto-commit timer (already done) 2. take a snapshot of the `subscriptionState.allConsumed` to be used on any commit of all consumed until the next poll (new, to fix the gap) With that, all commits will continue to retrieve the `allConsumed` in the background as they do now, and the fix is more about "when" to retrieve them. - before this PR the `allConsumed` are retrieved freely when needed to be used for committing (and that's wrong because there could be a fetch half-way) - with this fix, the `allConsumed` to commit would only be retrieved at the beginning of the poll loop before fetching (but in the background, via a `PollEvent` event). I'm thinking out loud here, could be missing stuff, let me know what do you think ;) -- 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
