Hi Andrew, AS2: I added a note for now. If others feel strongly about it, we can still add more administrative tools to the KIP - it should not change the overall story significantly.
AS8: "streams.group.assignor.name" sounds good to me to distinguish the config from class names. Not sure if I like the "default". To be consistent, we'd then have to call it `group.streams.default.session.timeout.ms` as well. I only added the `.name` on both broker and group level for now. AS10: Ah, I misread your comment, now I know what you meant. Good point, fixed (by Bruno). Cheers, Lucas On Fri, Jul 19, 2024 at 4:44 PM Andrew Schofield <andrew_schofi...@live.com> wrote: > > Hi Lucas, > I see that I hit send too quickly. One more comment: > > AS2: I think stating that there will be a `kafka-streams-group.sh` in a > future KIP is fine to keep this KIP focused. Personally, I would probably > put all of the gory details in this KIP, but then it’s not my KIP. A future > pointer is fine too. > > Thanks, > Andrew > > > > On 19 Jul 2024, at 13:46, Lucas Brutschy <lbruts...@confluent.io.INVALID> > > wrote: > > > > Hi Andrew, > > > > thanks for getting the discussion going! Here are my responses. > > > > AS1: Good point, done. > > > > AS2: We were planning to add more administrative tools to the > > interface in a follow-up KIP, to not make this KIP too large. If > > people think that it would help to understand the overall picture if > > we already add something like `kafka-streams-groups.sh`, we will do > > that. I also agree that we should address how this relates to > > KIP-1043, we'll add it. > > > > AS3: Good idea, that's more consistent with `assigning` and `reconciling` > > etc. > > > > AS4: Thanks, Fixed. > > > > AS5: Good catch. This was supposed to mean that we require CREATE on > > cluster or CREATE on all topics, not both. Fixed. > > > > AS6: Thanks, Fixed. > > > > AS7. Thanks, Fixed. > > > > AS8: I think this works a bit different in this KIP than in consumer > > groups. KIP-848 lets the members vote for a preferred assignor, and > > the broker-side assignor is picked by majority vote. The > > `group.consumer.assignors` specifies the list of assignors that are > > supported on the broker, and is configurable because the interface is > > pluggable. In this KIP, the task assignor is not voted on by members > > but configured on the broker-side. `group.streams.assignor` is used > > for this, and uses a specific name. If we'll make the task assignor > > pluggable on the broker-side, we'd introduce a separate config > > `group.streams.assignors`, which would indeed be a list of class > > names. I think there is no conflict here, the two configurations serve > > different purposes. The only gripe I'd have here is naming as > > `group.streams.assignor` and `group.streams.assignors` would be a bit > > similar, but I cannot really think of a better name for > > `group.streams.assignor`, so I'd probably rather introduce > > `group.streams.assignors` as `group.streams.possible_assignors` or > > something like that. > > > > AS9: I added explanations for the various record types. Apart from the > > new topology record, and the partition metadata (which is based on the > > topology and can only be created once we have a topology initialized) > > the lifecycle for the records is basically identical as in KIP-848. > > > > AS10: In the consumer offset topic, the version in the key is used to > > differentiate different schema types with the same content. So the > > keys are not versioned, but the version field is "abused" as a type > > tag. This is the same in KIP-848, we followed it for consistency. > > > > Cheers, > > Lucas > > > > > > On Thu, Jul 18, 2024 at 1:27 PM Andrew Schofield > > <andrew_schofi...@live.com> wrote: > >> > >> 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 > >> >
