Hi Lucas and Bruno, Thanks for the great KIP.
I've read through the document and have some initial comments. AS1: I suppose that there is a new o.a.k.common.GroupType.STREAMS enumeration constant. This is a change to the public interface and should be called out. AS2: Since streams groups are no longer consumer groups, how does the user manipulate them, observe lag and so on? Will you add `kafka-streams-groups.sh` or extend `kafka-streams-application-reset.sh`? Of course, KIP-1043 can easily be extended to support streams groups, but that only lets the user see the groups, not manipulate them. AS3: I wonder whether the streams group state of UNINITIALIZED would be better expressed as INITIALIZING. AS4: In StreamsGroupInitializeRequest, Topology[].SourceTopicRegex should be nullable. AS5: Why does StreamsGroupInitialize require CREATE permission on the cluster resource? I imagine that this is one of the ways that the request might be granted permission to create the StateChangelogTopics and RepartitionSourceTopics, but if it is granted permission to create those topics with specific ACLs, would CREATE on the cluster resource still be required? AS6: StreamsGroupInitialize can also fail with TOPIC_AUTHORIZATION_FAILED and (subject to AS5) CLUSTER_AUTHORIZATION_FAILED. AS7: A tiny nit. You've used TopologyID (capitals) in StreamsGroupHeartbeatRequest and a few others, but in all other cases the fields which are ids are spelled Id. I suggest TopologyId. Also, "interal" is probably meant to be "interval”. AS8: For consumer groups, the `group.consumer.assignors` configuration is a list of class names. The assignors do have names too, but the configuration which enables them is in terms of class names. I wonder whether the broker’s group.streams.assignor could actually be `group.streams.assignors` and specified as a list of the class names of the supplied assignors. I know you're not supporting other assignors yet, but when you do, I expect you would prefer to have used class names from the start. The use of assignor names in the other places looks good to me. AS9: I'd find it really helpful to have a bit of a description about the purpose and lifecycle of the 9 record types you've introduced on the __consumer_offsets topic. I did a cursory review but without really understanding what's written when, I can't do a thorough job of reviewing. AS10: In the definitions of the record keys, such as StreamsGroupCurrentMemberAssignmentKey, the versions of the fields must match the versions of the types. Thanks, Andrew > On 12 Jul 2024, at 09:04, Lucas Brutschy <lbruts...@confluent.io.INVALID> > wrote: > > Hi all, > > I would like to start a discussion thread on KIP-1071: Streams > Rebalance Protocol. With this KIP, we aim to bring the principles laid > down by KIP-848 to Kafka Streams, to make rebalances more reliable and > scalable, and make Kafka Streams overall easier to deploy and operate. > The KIP proposed moving the assignment logic to the broker, and > introducing a dedicated group type and dedicated RPCs for Kafka > Streams. > > The KIP is here: > https://cwiki.apache.org/confluence/display/KAFKA/KIP-1071%3A+Streams+Rebalance+Protocol > > This is joint work with Bruno Cadonna. > > Please take a look and let us know what you think. > > Best, > Lucas
