dajac commented on code in PR #17981: URL: https://github.com/apache/kafka/pull/17981#discussion_r1867428898
########## group-coordinator/src/main/java/org/apache/kafka/coordinator/group/GroupStore.java: ########## @@ -0,0 +1,307 @@ +/* Review Comment: @aliehsaeedii Thanks for working on this! I have a few high level comments regarding the `GroupStore`. I want us to agree on what we put and don't put in the store. * In my mind, the store should hold the state of all the groups. I think that we all agree on this. * As it holds the state of all groups, I am also tempted by moving all the replay methods to the store as they are the ones updating the state. Is it something that you have considered? * I think that the store should only have methods to query the state (e.g. get a group, list all groups, etc.). All the other methods (e.g. `validateDeleteGroup`, `maybeDeleteGroup`, `createGroupTombstoneRecords`, etc should not be here in my opinion. * The API specific methods should resides outside of the store. e.g. `listGroups` is directly linked to the implementation of the admin API, hence it may be better to have it in one of the other managers or in a manager responsible for the admin APIs. * The store does not need a reference to a MetadataImage. -- 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
