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

Reply via email to