Hello Sagar, It would be great if you could come back with some analysis on how to implement the Connect side integration with the new protocol; so far besides leveraging on the new "protocol type" we did not yet think through the Connect side implementations. For Streams here's a draft of integration plan: https://docs.google.com/document/d/17PNz2sGoIvGyIzz8vLyJTJTU2rqnD_D9uHJnH9XARjU/edit#heading=h.pdgirmi57dvn just FYI for your analysis on Connect.
On Tue, Jul 19, 2022 at 10:48 PM Sagar <sagarmeansoc...@gmail.com> wrote: > Hi David, > > Thank you for your response. The reason I thought connect can also fit into > this new scheme is that even today the connect uses a WorkerCoordinator > extending from AbstractCoordinator to empower rebalances of > tasks/connectors. The WorkerCoordinator sets the protocolType() to connect > and uses the metadata() method by plumbing into JoinGroupRequestProtocol. > > I think the changes to support connect would be similar at a high level to > the changes in streams mainly because of the Client side assignors being > used in both. At an implementation level, we might need to make a lot of > changes to get onto this new assignment protocol like enhancing the > JoinGroup request/response and SyncGroup and using ConsumerGroupHeartbeat > API etc again on similar lines to streams (or there might be deviations). I > would try to perform a detailed analysis of the same and we can have a > separate discussion thread for that as that would derail this discussion > thread. Let me know if that sounds good to you. > > Thanks! > Sagar. > > > > On Fri, Jul 15, 2022 at 5:47 PM David Jacot <dja...@confluent.io.invalid> > wrote: > > > Hi Sagar, > > > > Thanks for your comments. > > > > 1) Yes. That refers to `Assignment#error`. Sure, I can mention it. > > > > 2) The idea is to transition C from his current assignment to his > > target assignment when he can move to epoch 3. When that happens, the > > member assignment is updated and persisted with all its assigned > > partitions even if they are not all revoked yet. In other words, the > > member assignment becomes the target assignment. This is basically an > > optimization to avoid having to write all the changes to the log. The > > examples are based on the persisted state so I understand the > > confusion. Let me see if I can improve this in the description. > > > > 3) Regarding Connect, it could reuse the protocol with a client side > > assignor if it fits in the protocol. The assignment is about > > topicid-partitions + metadata, could Connect fit into this? > > > > Best, > > David > > > > On Fri, Jul 15, 2022 at 1:55 PM Sagar <sagarmeansoc...@gmail.com> wrote: > > > > > > Hi David, > > > > > > Thanks for the KIP. I just had minor observations: > > > > > > 1) In the Assignment Error section in Client Side mode Assignment > > process, > > > you mentioned => `In this case, the client side assignor can return an > > > error to the group coordinator`. In this case are you referring to the > > > Assignor returning an AssignmentError that's listed down towards the > end? > > > If yes, do you think it would make sense to mention this explicitly > here? > > > > > > 2) In the Case Studies section, I have a slight confusion, not sure if > > > others have the same. Consider this step: > > > > > > When B heartbeats, the group coordinator transitions him to epoch 3 > > because > > > B has no partitions to revoke. It persists the change and reply. > > > > > > - Group (epoch=3) > > > - A > > > - B > > > - C > > > - Target Assignment (epoch=3) > > > - A - partitions=[foo-0] > > > - B - partitions=[foo-2] > > > - C - partitions=[foo-1] > > > - Member Assignment > > > - A - epoch=2, partitions=[foo-0, foo-1] > > > - B - epoch=3, partitions=[foo-2] > > > - C - epoch=3, partitions=[foo-1] > > > > > > When C heartbeats, it transitions to epoch 3 but cannot get foo-1 yet. > > > > > > Here,it's mentioned that member C can't get the foo-1 partition yet, > but > > > based on the description above, it seems it already has it. Do you > think > > it > > > would be better to remove it and populate it only when it actually gets > > it? > > > I see this in a lot of other places, so have I understood it > incorrectly > > ? > > > > > > > > > Regarding connect , it might be out of scope of this discussion, but > from > > > what I understood it would probably be running in client side assignor > > mode > > > even on the new rebalance protocol as it has its own Custom > > Assignors(Eager > > > and IncrementalCooperative). > > > > > > Thanks! > > > > > > Sagar. > > > > > > > > > > > > > > > > > > > > > On Fri, Jul 15, 2022 at 5:00 PM David Jacot > <dja...@confluent.io.invalid > > > > > > wrote: > > > > > > > Thanks Hector! Our goal is to move forward with specialized API > > > > instead of relying on one generic API. For Connect, we can apply the > > > > exact same pattern and reuse/share the core implementation on the > > > > server side. For the schema registry, I think that we should consider > > > > having a tailored API to do simple membership/leader election. > > > > > > > > Best, > > > > David > > > > > > > > On Fri, Jul 15, 2022 at 10:22 AM Ismael Juma <ism...@juma.me.uk> > > wrote: > > > > > > > > > > Three quick comments: > > > > > > > > > > 1. Regarding java.util.regex.Pattern vs com.google.re2j.Pattern, we > > > > should > > > > > document the differences in more detail before deciding one way or > > > > another. > > > > > That said, if people pass java.util.regex.Pattern, they expect > their > > > > > semantics to be honored. If we are doing something different, then > we > > > > > should consider adding an overload with our own Pattern class (I > > don't > > > > > think we'd want to expose re2j's at this point). > > > > > 2. Regarding topic ids, any major new protocol should integrate > fully > > > > with > > > > > it and should handle the topic recreation case correctly. That's > the > > main > > > > > part we need to handle. I agree with David that we'd want to add > > topic > > > > ids > > > > > to the relevant protocols that don't have it yet and that we can > > probably > > > > > focus on the internals versus adding new APIs to the Java Consumer > > > > (unless > > > > > we find that adding new APIs is required for reasonable semantics). > > > > > 3. I am still not sure about the coordinator storing the configs. > > It's > > > > > powerful for configs to be centralized in the metadata log for > > various > > > > > reasons (auditability, visibility, consistency, etc.). Similarly, I > > am > > > > not > > > > > sure about automatically deleting configs in a way that they cannot > > be > > > > > recovered. A good property for modern systems is to minimize the > > number > > > > of > > > > > unrecoverable data loss scenarios. > > > > > > > > > > Ismael > > > > > > > > > > On Wed, Jul 13, 2022 at 3:47 PM David Jacot > > <dja...@confluent.io.invalid > > > > > > > > > > wrote: > > > > > > > > > > > Thanks Guozhang. My answers are below: > > > > > > > > > > > > > 1) the migration path, especially the last step when clients > > flip the > > > > > > flag > > > > > > > to enable the new protocol, in which we would have a window > where > > > > both > > > > > > new > > > > > > > protocols / rpcs and old protocols / rpcs are used by members > of > > the > > > > same > > > > > > > group. How the coordinator could "mimic" the old behavior while > > > > using the > > > > > > > new protocol is something we need to present about. > > > > > > > > > > > > Noted. I just published a new version of KIP which includes more > > > > > > details about this. See the "Supporting Online Consumer Group > > Upgrade" > > > > > > and the "Compatibility, Deprecation, and Migration Plan". I think > > that > > > > > > I have to think through a few cases now but the overall idea and > > > > > > mechanism should be understandable. > > > > > > > > > > > > > 2) the usage of topic ids. So far as KIP-516 the topic ids are > > only > > > > used > > > > > > as > > > > > > > part of RPCs and admin client, but they are not exposed via any > > > > public > > > > > > APIs > > > > > > > to consumers yet. I think the question is, first should we let > > the > > > > > > consumer > > > > > > > client to be maintaining the names -> ids mapping itself to > fully > > > > > > leverage > > > > > > > on all the augmented existing RPCs and the new RPCs with the > > topic > > > > ids; > > > > > > and > > > > > > > secondly, should we ever consider exposing the topic ids in the > > > > consumer > > > > > > > public APIs as well (both subscribe/assign, as well as in the > > > > rebalance > > > > > > > listener for cases like topic deletion-and-recreation). > > > > > > > > > > > > a) Assuming that we would include converting all the offsets > > related > > > > > > RPCs to using topic ids in this KIP, the consumer would be able > to > > > > > > fully operate with topic ids. That being said, it still has to > > provide > > > > > > the topics names in various APIs so having a mapping in the > > consumer > > > > > > seems inevitable to me. > > > > > > b) I don't have a strong opinion on this. Here I wonder if this > > goes > > > > > > beyond the scope of this KIP. I would rather focus on the > internals > > > > > > here and we can consider this separately if we see value in doing > > it. > > > > > > > > > > > > Coming back to Ismael's point about using topic ids in the > > > > > > ConsumerGroupHeartbeatRequest, I think that there is one > advantage > > in > > > > > > favour of it. The consumer will have the opportunity to validate > > that > > > > > > the topics exists before passing them into the group rebalance > > > > > > protocol. Obviously, the coordinator will also notice it but it > > does > > > > > > not really have a way to reject an invalid topic in the response. > > > > > > > > > > > > > I'm agreeing with David on all other minor questions except for > > the > > > > > > > `subscribe(Pattern)` question: personally I think it's not > > necessary > > > > to > > > > > > > deprecate the subscribe API with Pattern, but instead we still > > use > > > > > > Pattern > > > > > > > while just documenting that our subscription may be rejected by > > the > > > > > > server. > > > > > > > Since the incompatible case is a very rare scenario I felt > using > > an > > > > > > > overloaded `String` based subscription may be more vulnerable > to > > > > various > > > > > > > invalid regexes. > > > > > > > > > > > > That could work. I have to look at the differences between the > two > > > > > > engines to better understand the potential issues. My > > understanding is > > > > > > that would work for all the basic regular expressions. The > > differences > > > > > > between the two are mainly about the various character classes. I > > > > > > wonder what other people think about this. > > > > > > > > > > > > Best, > > > > > > David > > > > > > > > > > > > On Tue, Jul 12, 2022 at 11:28 PM Guozhang Wang < > wangg...@gmail.com > > > > > > > wrote: > > > > > > > > > > > > > > Thanks David! I think on the high level there are two meta > > points we > > > > need > > > > > > > to concretize a bit more: > > > > > > > > > > > > > > 1) the migration path, especially the last step when clients > > flip the > > > > > > flag > > > > > > > to enable the new protocol, in which we would have a window > where > > > > both > > > > > > new > > > > > > > protocols / rpcs and old protocols / rpcs are used by members > of > > the > > > > same > > > > > > > group. How the coordinator could "mimic" the old behavior while > > > > using the > > > > > > > new protocol is something we need to present about. > > > > > > > 2) the usage of topic ids. So far as KIP-516 the topic ids are > > only > > > > used > > > > > > as > > > > > > > part of RPCs and admin client, but they are not exposed via any > > > > public > > > > > > APIs > > > > > > > to consumers yet. I think the question is, first should we let > > the > > > > > > consumer > > > > > > > client to be maintaining the names -> ids mapping itself to > fully > > > > > > leverage > > > > > > > on all the augmented existing RPCs and the new RPCs with the > > topic > > > > ids; > > > > > > and > > > > > > > secondly, should we ever consider exposing the topic ids in the > > > > consumer > > > > > > > public APIs as well (both subscribe/assign, as well as in the > > > > rebalance > > > > > > > listener for cases like topic deletion-and-recreation). > > > > > > > > > > > > > > I'm agreeing with David on all other minor questions except for > > the > > > > > > > `subscribe(Pattern)` question: personally I think it's not > > necessary > > > > to > > > > > > > deprecate the subscribe API with Pattern, but instead we still > > use > > > > > > Pattern > > > > > > > while just documenting that our subscription may be rejected by > > the > > > > > > server. > > > > > > > Since the incompatible case is a very rare scenario I felt > using > > an > > > > > > > overloaded `String` based subscription may be more vulnerable > to > > > > various > > > > > > > invalid regexes. > > > > > > > > > > > > > > > > > > > > > Guozhang > > > > > > > > > > > > > > On Tue, Jul 12, 2022 at 5:23 AM David Jacot > > > > <dja...@confluent.io.invalid > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > Hi Ismael, > > > > > > > > > > > > > > > > Thanks for your feedback. Let me answer your questions > inline. > > > > > > > > > > > > > > > > > 1. I think it's premature to talk about target versions for > > > > > > deprecation > > > > > > > > and > > > > > > > > > removal of the existing group protocol. Unlike KRaft, this > > > > affects a > > > > > > core > > > > > > > > > client protocol and hence deprecation/removal will be > heavily > > > > > > dependent > > > > > > > > on > > > > > > > > > how quickly applications migrate to the new protocol. > > > > > > > > > > > > > > > > That makes sense. I will remove it. > > > > > > > > > > > > > > > > > 2. The KIP says we intend to release this in 4.x, but it > > wasn't > > > > made > > > > > > > > clear > > > > > > > > > why. If we added that as a way to estimate when we'd > > deprecate > > > > and > > > > > > remove > > > > > > > > > the group protocol, I also suggest removing this part. > > > > > > > > > > > > > > > > Let me explain my reasoning. As explained, I plan to rewrite > > the > > > > group > > > > > > > > coordinator in Java while we implement the new protocol. This > > means > > > > > > > > that the internals will be slightly different (e.g. threading > > > > model). > > > > > > > > Therefore, I wanted to tighten the switch from the old group > > > > > > > > coordinator to the new group coordinator to a major release. > > The > > > > > > > > alternative would be to use a flag to do the switch instead > of > > > > relying > > > > > > > > on the software upgrade. > > > > > > > > > > > > > > > > > 3. We need to flesh out the details of the migration story. > > It > > > > sounds > > > > > > > > like > > > > > > > > > we're saying we will support online migrations. Is that > > correct? > > > > We > > > > > > > > should > > > > > > > > > explain this in detail. It could also be done as a separate > > KIP, > > > > if > > > > > > it's > > > > > > > > > easier. > > > > > > > > > > > > > > > > Yes, we will support online migrations for the group. That > > means > > > > that > > > > > > > > a group using the old protocol will be able to switch to the > > new > > > > > > > > protocol. > > > > > > > > > > > > > > > > Let me briefly explain how that will work though. It is > > basically a > > > > > > > > four step process: > > > > > > > > > > > > > > > > 1. The cluster must be upgraded or rolled to a software > > supporting > > > > the > > > > > > > > new group coordinator. Both the old and the new coordinator > > will > > > > > > > > support the old protocol and rely on the same persisted > > metadata so > > > > > > > > they can work together. This point is an offline migration. > We > > > > cannot > > > > > > > > do this one live because it would require shutting down the > > current > > > > > > > > coordinator and starting up the new one and that would cause > > > > > > > > unavailabilities. > > > > > > > > 2. The cluster's metadata version/IBP must be upgraded to X > in > > > > order > > > > > > > > to enable the new protocol. This cannot be done before 1) is > > > > > > > > terminated because the old coordinator doesn't support the > new > > > > > > > > protocol. > > > > > > > > 3. The consumers must be upgraded to a version supporting the > > > > online > > > > > > > > migration (must have KIP-792). If the consumer is already > > there. > > > > > > > > Nothing must be done at this point. > > > > > > > > 4. The consumers must be rolled with the feature flag turned > > on. > > > > The > > > > > > > > consumer group is automatically converted when the first > > consumer > > > > > > > > using the new protocol joins the group. While the members > > using the > > > > > > > > old protocol are being upgraded, the old protocol is proxied > > into > > > > the > > > > > > > > new one. > > > > > > > > > > > > > > > > Let me clarify all of this in the KIP. > > > > > > > > > > > > > > > > > 4. I am happy that we are pushing the pattern subscriptions > > to > > > > the > > > > > > > > server, > > > > > > > > > but it seems like there could be some tricky compatibility > > > > issues. > > > > > > Will > > > > > > > > we > > > > > > > > > have a mechanism for users to detect that they need to > update > > > > their > > > > > > regex > > > > > > > > > before switching to the new protocol? > > > > > > > > > > > > > > > > I think that I am a bit more optimistic than you on this > > point. I > > > > > > > > believe that the majority of the cases are simple regexes > which > > > > should > > > > > > > > work with the new engine. The coordinator will verify the > regex > > > > anyway > > > > > > > > and reject the consumer if the regex is not valid. Coming > back > > to > > > > the > > > > > > > > migration path, in the worst case, the first upgraded > consumer > > > > joining > > > > > > > > the group will be rejected. This should be used as the last > > > > defence, I > > > > > > > > would say. > > > > > > > > > > > > > > > > One way for customers to validate their regex before > upgrading > > > > their > > > > > > > > prod would be to test them with another group. For instance, > > that > > > > > > > > could be done in a pre-prod environment. Another way would be > > to > > > > > > > > extend the consumer-group tool to provide a regex validation > > > > > > > > mechanism. Would this be enough in your opinion? > > > > > > > > > > > > > > > > > 5. Related to the last question, will the Java client allow > > the > > > > > > users to > > > > > > > > > stick with the current regex engine for compatibility > > reasons? > > > > For > > > > > > > > example, > > > > > > > > > it may be handy to keep using client based regex at first > to > > keep > > > > > > > > > migrations simple and then migrate to server based regexes > > as a > > > > > > second > > > > > > > > step. > > > > > > > > > > > > > > > > I understand your point but I am concerned that this would > > allow > > > > users > > > > > > > > to actually stay in this mode. That would go against our goal > > of > > > > > > > > simplifying the client because we would have to continue > > monitoring > > > > > > > > the metadata on the client side. I would rather not do this. > > > > > > > > > > > > > > > > > 6. When we say that the group coordinator will be > > responsible for > > > > > > storing > > > > > > > > > the configurations and that the configurations will be > > deleted > > > > when > > > > > > the > > > > > > > > > group is deleted. Will a transition to DEAD trigger > deletion > > of > > > > > > > > > configurations? > > > > > > > > > > > > > > > > That's right. The configurations will be deleted when the > > group is > > > > > > > > deleted. They go together. > > > > > > > > > > > > > > > > > 7. Will the choice to store the configs in the group > > coordinator > > > > > > make it > > > > > > > > > harder to list all cluster configs and their values? > > > > > > > > > > > > > > > > I don't think so. The group configurations are overrides of > > cluster > > > > > > > > configs. If you want to know all the overrides though, you > > would > > > > have > > > > > > > > to ask all the group coordinators. You cannot rely on the > > metadata > > > > log > > > > > > > > for instance. > > > > > > > > > > > > > > > > > 8. How would someone configure a group before starting the > > > > consumers? > > > > > > > > Have > > > > > > > > > we considered allowing the explicit creation of groups? > > > > > > Alternatively, > > > > > > > > the > > > > > > > > > configs could be decoupled from the group lifecycle. > > > > > > > > > > > > > > > > Yes. The group will be automatically created in this case. > > However, > > > > > > > > the configs will be lost after the retention period of the > > group > > > > > > > > passes. > > > > > > > > > > > > > > > > > 9. Will the Consumer.subscribe method for the Java client > > still > > > > take > > > > > > a > > > > > > > > > `java.util.regex.Pattern` of do we have to introduce an > > overload? > > > > > > > > > > > > > > > > That's a very group question. I forgot about that one. As the > > > > > > > > `java.util.regex.Pattern` is not fully compatible with the > > engine > > > > that > > > > > > > > we plan to use, it might be better to deprecate it and use an > > > > overload > > > > > > > > which takes a string. We would rely on the server side > > validation. > > > > > > > > During the migration, I think that we could still try to > > toString > > > > the > > > > > > > > regex and use it. That should work, I think, in the majority > > of the > > > > > > > > cases. > > > > > > > > > > > > > > > > > 10. I agree with Justine that we should be clearer about > the > > > > reason > > > > > > to > > > > > > > > > switch to IBP/metadata.version from the feature flag. Maybe > > we > > > > mean > > > > > > that > > > > > > > > we > > > > > > > > > can switch the default for the feature flag to true based > on > > the > > > > > > > > > metadata.version once we want to make it the default. > > > > > > > > > > > > > > > > My plan was to use that feature flag mainly during the > > development > > > > > > > > phase. I should not have mentioned it, I think, because we > > could > > > > use > > > > > > > > an internal config for it. > > > > > > > > > > > > > > > > > 11. Some of the protocol APIs don't mention the required > > ACLs, it > > > > > > would > > > > > > > > be > > > > > > > > > good to add that for consistency. > > > > > > > > > > > > > > > > Noted. > > > > > > > > > > > > > > > > > 12. It is a bit odd that ConsumerGroupHeartbeat requires > > "Read > > > > Group" > > > > > > > > even > > > > > > > > > though it seems to do more than reading. > > > > > > > > > > > > > > > > I agree. This is how the current protocol works though. We > only > > > > > > > > require "Read Group" to join a group. We could consider > > changing > > > > this > > > > > > > > but I am not sure that it is worth it. > > > > > > > > > > > > > > > > > 13. How is topic recreation handled by the consumer with > the > > new > > > > > > group > > > > > > > > > protocol? It would be good to have a section on this. > > > > > > > > > > > > > > > > Noted. From a protocol perspective, the new topic will have a > > new > > > > > > > > topic id so it will treat it like a topic with a different > > name. > > > > The > > > > > > > > only issue is that the fetch/commit offsets APIs do not > support > > > > topic > > > > > > > > IDs so the consumer would reuse the offsets based on the > same. > > I > > > > think > > > > > > > > that we should update those APIs as well in order to be > > consistent > > > > end > > > > > > > > to end. That would strengthen the semantics of the consumer. > > > > > > > > > > > > > > > > > 14. The KIP mentions we will write the new coordinator in > > Java. > > > > Even > > > > > > > > though > > > > > > > > > this is an implementation detail, do we plan to have a new > > gradle > > > > > > module > > > > > > > > > for it? > > > > > > > > > > > > > > > > Yes. > > > > > > > > > > > > > > > > > 15. Do we have a scalability goal when it comes to how many > > > > members > > > > > > the > > > > > > > > new > > > > > > > > > group protocol can support? > > > > > > > > > > > > > > > > We don't have numbers at the moment. The protocol should > > support > > > > 1000s > > > > > > > > of members per group. We will measure this when we have a > first > > > > > > > > implementation. Note that we might have other bottlenecks > down > > the > > > > > > > > road (e.g. offset commits). > > > > > > > > > > > > > > > > > 16. Did we consider having SubscribedTopidIds instead of > > > > > > > > > SubscribedTopicNames in ConsumerGroupHeartbeatRequest? Is > the > > > > idea > > > > > > that > > > > > > > > > since we have to resolve the regex on the server, we can do > > the > > > > same > > > > > > for > > > > > > > > > the topic name? The difference is that sending the regex is > > more > > > > > > > > efficient > > > > > > > > > whereas sending the topic names is less efficient. > > Furthermore, > > > > > > delete > > > > > > > > and > > > > > > > > > recreation is easier to handle if we have topic ids. > > > > > > > > > > > > > > > > The idea was to consolidate the metadata lookup on the server > > for > > > > both > > > > > > > > paths but I do agree with your point. As a second though, > using > > > > topic > > > > > > > > ids may be better here for the delete and recreation case. > > Also, I > > > > > > > > suppose that we may allow users to subscribe with topic ids > in > > the > > > > > > > > future because that is the only way to be really robust to > > topic > > > > > > > > re-creation. > > > > > > > > > > > > > > > > Best, > > > > > > > > David > > > > > > > > > > > > > > > > On Tue, Jul 12, 2022 at 1:38 PM David Jacot < > > dja...@confluent.io> > > > > > > wrote: > > > > > > > > > > > > > > > > > > Hi Justine, > > > > > > > > > > > > > > > > > > Thanks for your comments. Please find my answers below. > > > > > > > > > > > > > > > > > > - Yes, the new protocol relies on topic IDs with the > > exception > > > > of the > > > > > > > > > topic names based in the ConsumerGroupHeartbeatRequest. I > am > > not > > > > sure > > > > > > > > > if using topic names is the right call here. I need to > think > > > > about it > > > > > > > > > a little more. Obviously, the KIP does not change the > > > > fetch/commit > > > > > > > > > offsets RPCs to use topic IDs. This may be something that > we > > > > should > > > > > > > > > include though as it would give better overall guarantee in > > the > > > > > > > > > producer. > > > > > > > > > - You're right. I think that I should not have mentioned > this > > > > flag at > > > > > > > > > all. I will remove it. We can use an internal configuration > > while > > > > > > > > > developing the feature. > > > > > > > > > - Both cluster types will be supported. The change is > > > > orthogonal. The > > > > > > > > > only requirement is that the cluster uses topic IDs. > > > > > > > > > > > > > > > > > > Best, > > > > > > > > > David > > > > > > > > > > > > > > > > > > On Mon, Jul 11, 2022 at 9:53 PM Guozhang Wang < > > > > wangg...@gmail.com> > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > Hi Ismael, > > > > > > > > > > > > > > > > > > > > Thanks for the feedback. Here are some replies inlined > > below: > > > > > > > > > > > > > > > > > > > > On Sat, Jul 9, 2022 at 2:53 AM Ismael Juma < > > ism...@juma.me.uk> > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > Thanks for the KIP. This has the potential to be a > great > > > > > > > > improvement. A few > > > > > > > > > > > initial questions/comments: > > > > > > > > > > > > > > > > > > > > > > 1. I think it's premature to talk about target versions > > for > > > > > > > > deprecation and > > > > > > > > > > > removal of the existing group protocol. Unlike KRaft, > > this > > > > > > affects a > > > > > > > > core > > > > > > > > > > > client protocol and hence deprecation/removal will be > > heavily > > > > > > > > dependent on > > > > > > > > > > > how quickly applications migrate to the new protocol. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Yeah I agree with you. I think we can remove the proposed > > > > timeline > > > > > > in > > > > > > > > the > > > > > > > > > > `Compatibility, Deprecation, and Migration Plan` and > > instead > > > > just > > > > > > state > > > > > > > > > > that we will decide in the future about when we would > > > > deprecate old > > > > > > > > > > protocol and behaviors. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 2. The KIP says we intend to release this in 4.x, but > it > > > > wasn't > > > > > > made > > > > > > > > clear > > > > > > > > > > > why. If we added that as a way to estimate when we'd > > > > deprecate > > > > > > and > > > > > > > > remove > > > > > > > > > > > the group protocol, I also suggest removing this part. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > I think that's not specifically related to the > > > > deprecation/removal > > > > > > > > timeline > > > > > > > > > > plan, but it's more for client upgrades. I.e. the > > broker-side > > > > > > > > > > implementation may be done first, and then the client > side, > > > > and we > > > > > > > > would > > > > > > > > > > only mark it as "released" by the time clients > > implementations > > > > are > > > > > > > > done. At > > > > > > > > > > that time, to enable the feature the clients need to > first > > > > swap-in > > > > > > the > > > > > > > > > > bytecode with a rolling bounce and then set the flag > with a > > > > second > > > > > > > > rolling > > > > > > > > > > bounce, and hence we feel it's better to be released in a > > major > > > > > > > > version. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 3. We need to flesh out the details of the migration > > story. > > > > It > > > > > > > > sounds like > > > > > > > > > > > we're saying we will support online migrations. Is that > > > > correct? > > > > > > We > > > > > > > > should > > > > > > > > > > > explain this in detail. It could also be done as a > > separate > > > > KIP, > > > > > > if > > > > > > > > it's > > > > > > > > > > > easier. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Yes I think that's the part we can be more concrete about > > for > > > > sure > > > > > > (and > > > > > > > > > > this is related to your question 2) above). We will work > on > > > > making > > > > > > it > > > > > > > > more > > > > > > > > > > explicit in parallel as we solicit more feedback. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 4. I am happy that we are pushing the pattern > > subscriptions > > > > to > > > > > > the > > > > > > > > server, > > > > > > > > > > > but it seems like there could be some tricky > > compatibility > > > > > > issues. > > > > > > > > Will we > > > > > > > > > > > have a mechanism for users to detect that they need to > > update > > > > > > their > > > > > > > > regex > > > > > > > > > > > before switching to the new protocol? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Yes I think we need some tooling for non-java client > users > > to > > > > sort > > > > > > of > > > > > > > > > > "dry-run" the client before switching to the new > protocol. > > I > > > > do not > > > > > > > > have a > > > > > > > > > > specific idea on top of my head though, maybe others like > > @Matt > > > > > > > > Howlett can > > > > > > > > > > chime-in here? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 5. Related to the last question, will the Java client > > allow > > > > the > > > > > > > > users to > > > > > > > > > > > stick with the current regex engine for compatibility > > > > reasons? > > > > > > For > > > > > > > > example, > > > > > > > > > > > it may be handy to keep using client based regex at > > first to > > > > keep > > > > > > > > > > > migrations simple and then migrate to server based > > regexes > > > > as a > > > > > > > > second > > > > > > > > > > > step. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Honestly I have not thought about that for java clients, > > and > > > > we can > > > > > > > > discuss > > > > > > > > > > that. What kind of compatibility issues do you have in > > mind? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 6. When we say that the group coordinator will be > > > > responsible for > > > > > > > > storing > > > > > > > > > > > the configurations and that the configurations will be > > > > deleted > > > > > > when > > > > > > > > the > > > > > > > > > > > group is deleted. Will a transition to DEAD trigger > > deletion > > > > of > > > > > > > > > > > configurations? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Yes, since the DEAD state is an ending state (we would > only > > > > > > transit to > > > > > > > > that > > > > > > > > > > state when the group is EMPTY and also all of its > metadata > > are > > > > > > gone), > > > > > > > > once > > > > > > > > > > it's transited to DEAD this group would never be revived. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 7. Will the choice to store the configs in the group > > > > coordinator > > > > > > > > make it > > > > > > > > > > > harder to list all cluster configs and their values? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > That's a good question, and our thoughts are that the > > so-called > > > > > > "group > > > > > > > > > > configurations" are overrides of the cluster-level > > > > configurations > > > > > > > > > > customized per group so when an admin list cluster > configs > > it's > > > > > > okay to > > > > > > > > > > list just the cluster-level "defaults", not showing any > > > > per-group > > > > > > > > > > customizations. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 8. How would someone configure a group before starting > > the > > > > > > > > consumers? Have > > > > > > > > > > > we considered allowing the explicit creation of groups? > > > > > > > > Alternatively, the > > > > > > > > > > > configs could be decoupled from the group lifecycle. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > The configs can be created before the group itself as an > > > > > > independent > > > > > > > > entity > > > > > > > > > > --- of course, this requires the corresponding request to > > be > > > > > > routed to > > > > > > > > the > > > > > > > > > > right coordinator based on the group id --- the only > thing > > that > > > > > > > > differs is, > > > > > > > > > > when the group itself is gone we also check if there are > > any > > > > > > > > configuration > > > > > > > > > > entities related to that group and delete as well. > > > > > > > > > > > > > > > > > > > > Admittedly this indeed introduces an asymmetry on the > > creation > > > > / > > > > > > > > deletion > > > > > > > > > > lifecycles of the config entities, and we would like to > > hear > > > > > > everyone's > > > > > > > > > > feelings whether we should aim for symmetry i.e. totally > > > > decouple > > > > > > group > > > > > > > > > > configs and hence not delete them at all when the group > is > > > > gone, > > > > > > but > > > > > > > > always > > > > > > > > > > require explicit deletion operations by themselves. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 9. Will the Consumer.subscribe method for the Java > client > > > > still > > > > > > take > > > > > > > > a > > > > > > > > > > > `java.util.regex.Pattern` of do we have to introduce an > > > > overload? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > I think we do not need to introduce an overload, but I'm > > all > > > > ears > > > > > > if > > > > > > > > there > > > > > > > > > > may be some compatibility issues that we may overlook. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 10. I agree with Justine that we should be clearer > about > > the > > > > > > reason > > > > > > > > to > > > > > > > > > > > switch to IBP/metadata.version from the feature flag. > > Maybe > > > > we > > > > > > mean > > > > > > > > that we > > > > > > > > > > > can switch the default for the feature flag to true > > based on > > > > the > > > > > > > > > > > metadata.version once we want to make it the default. > > > > > > > > > > > > > > > > > > > > 11. Some of the protocol APIs don't mention the required > > ACLs, > > > > it > > > > > > > > would be > > > > > > > > > > > good to add that for consistency. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Ack, we can certainly do that. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 12. It is a bit odd that ConsumerGroupHeartbeat > requires > > > > "Read > > > > > > > > Group" even > > > > > > > > > > > though it seems to do more than reading. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > I had that thought myself as well, but in the end we > could > > not > > > > > > find a > > > > > > > > > > better alternative: adding Write Group seems an overkill > > here > > > > > > since we > > > > > > > > do > > > > > > > > > > not have it elsewhere (we only have Read / Delete and > > Describe > > > > on > > > > > > > > groups so > > > > > > > > > > far). Would like to hear others thoughts. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 13. How is topic recreation handled by the consumer > with > > the > > > > new > > > > > > > > group > > > > > > > > > > > protocol? It would be good to have a section on this. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > You mean with regex subscription right? Yes we can add a > > > > section > > > > > > about > > > > > > > > > > that, but basically the idea is that consumer would be > > totally > > > > > > > > agnostic in > > > > > > > > > > the new protocol as it's handled all by the brokers. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 14. The KIP mentions we will write the new coordinator > in > > > > Java. > > > > > > Even > > > > > > > > though > > > > > > > > > > > this is an implementation detail, do we plan to have a > > new > > > > gradle > > > > > > > > module > > > > > > > > > > > for it? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > We have not thought about that. But I think the answer > > should > > > > be > > > > > > yes. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 15. Do we have a scalability goal when it comes to how > > many > > > > > > members > > > > > > > > the new > > > > > > > > > > > group protocol can support? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Within a group, I think we should shoot for 1000s of > > members. > > > > But > > > > > > that > > > > > > > > > > scalability goals also depend on the offset management > > (commit, > > > > > > fetch) > > > > > > > > > > capabilities of the coordinator which we did not cover in > > this > > > > > > KIP, so > > > > > > > > it's > > > > > > > > > > hard to give a number that applies universally. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 16. Did we consider having SubscribedTopidIds instead > of > > > > > > > > > > > SubscribedTopicNames in ConsumerGroupHeartbeatRequest? > > Is the > > > > > > idea > > > > > > > > that > > > > > > > > > > > since we have to resolve the regex on the server, we > can > > do > > > > the > > > > > > same > > > > > > > > for > > > > > > > > > > > the topic name? The difference is that sending the > regex > > is > > > > more > > > > > > > > efficient > > > > > > > > > > > whereas sending the topic names is less efficient. > > > > Furthermore, > > > > > > > > delete and > > > > > > > > > > > recreation is easier to handle if we have topic ids. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > The main reason to still let the clients send names is to > > keep > > > > the > > > > > > > > > > reasoning of names -> ids on the broker / admin client > > only. > > > > Note > > > > > > that > > > > > > > > > > although we added topic id in KIP-516, we never > > implemented the > > > > > > logic > > > > > > > > on > > > > > > > > > > consumer/producers leveraging the related newer versioned > > RPCs, > > > > > > > > instead we > > > > > > > > > > just set the topic id as empty UUID. We want to keep the > > > > > > > > consumer/producer > > > > > > > > > > to be thin and only delegate the reasoning on broker and > > > > > > potentially > > > > > > > > admin > > > > > > > > > > clients. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Thanks, > > > > > > > > > > > Ismael > > > > > > > > > > > > > > > > > > > > > > On Wed, Jul 6, 2022 at 10:45 AM David Jacot > > > > > > > > <dja...@confluent.io.invalid> > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > Hi all, > > > > > > > > > > > > > > > > > > > > > > > > I would like to start a discussion thread on KIP-848: > > The > > > > Next > > > > > > > > > > > > Generation of the Consumer Rebalance Protocol. With > > this > > > > KIP, > > > > > > we > > > > > > > > aim > > > > > > > > > > > > to make the rebalance protocol (for consumers) more > > > > reliable, > > > > > > more > > > > > > > > > > > > scalable, easier to implement for clients, and easier > > to > > > > debug > > > > > > for > > > > > > > > > > > > operators. > > > > > > > > > > > > > > > > > > > > > > > > The KIP is here: > > > > https://cwiki.apache.org/confluence/x/HhD1D. > > > > > > > > > > > > > > > > > > > > > > > > Please take a look and let me know what you think. > > > > > > > > > > > > > > > > > > > > > > > > Best, > > > > > > > > > > > > David > > > > > > > > > > > > > > > > > > > > > > > > PS: I will be away from July 18th to August 8th. That > > gives > > > > > > you a > > > > > > > > bit > > > > > > > > > > > > of time to read and digest this long KIP. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > > > > > -- Guozhang > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > > -- Guozhang > > > > > > > > > > > > > > > -- -- Guozhang
