Hello Luke, Thanks for the detailed feedback! I tried to incorporate / answer some of them inline below.
On Thu, Jul 21, 2022 at 6:39 AM Luke Chen <show...@gmail.com> wrote: > Hi David, > > So excited to see the consumer protocol will be renewed! > Thanks for David, Guozhang, and Jason for creating this great proposal! > > Some comments about the protocol: > 1. Will the member reconciling jump to the latest target assignment? For > example, member epoch are A:1, B:2, but now the group is in target > assignment epoch 4. Will the members jump to the target assignment result > in epoch 4 directly, or they should first sync to epoch 2, then 3, and 4? I > tried to find out if there's any problem if we jump to 4, but I can't. So, > I think we'll go to epoch 4 directly, right? > > The short answer is "yes" :) But the a bit longer answer is that, such reconciliation logic that may skip some epochs are controlled by the brokers, not the clients. The clients should be simply dumb and takes in the indicated (delta) assignment as long as its epoch is no smaller than its current one. The brokers, having full knowledge of where everyone is at the moment, and the latest target assignment, would decide which delta assignment to send, step by step, and which assignment could be skipped. > 2. For the ConsumerGroupPrepareAssignment and > ConsumerGroupInstallAssignment API, when member epoch doesn't match, it'll > retry (and hope) in next heartbeat response, the member epoch will be > bumped. But in ConsumerGroupHeartbeat, Upon receiving the > FENCED_MEMBER_EPOCH error, the consumer abandon all its partitions and > rejoins with the same member id and the epoch 0, unless the partitions > owned by the members are a subset of the target partitions. I'm wondering > why can't we return the latest member epoch to the member in > ConsumerGroupHeartbeat response no matter if the owned partitions are > subset of target partitions or not. After all, the member is still one of > the group member, the assignment for the member is still valid, right? This > way, the ConsumerGroupPrepareAssignment and ConsumerGroupInstallAssignment > API can piggyback it to retry in next heartbeat response. WDYT? > > The semantics of these two classes of requests are different: for ConsumerGroupPrepareAssignment / ConsumerGroupInstallAssignment it's not really indicating the consumer is actually "fenced", but only to indicate "your information is probably stale, so just stay put and wait for the renewed info". Note that these two requests are only for clients to try collecting information or making a change to the group assignment, so even fai that request does not mean the member is no longer part of the group. BTW that reminds me for these two requests maybe we should use a different error code than FENCED_MEMBER_EPOCH since it's a bit confusing, maybe "STALE_MEMBER_EPOCH"? For ConsumerGroupHeartbeat, the fencing purpose is only to make sure that any partitions that are assigned to others should not be retained at the "fenced member". So I think we can make the assignment response in ConsumerGroupHeartbeat idempotent, and only return the fatal error code if the consumer's "owned partitions" has any overlaps with other members' current assignment, and otherwise we just proceed with the new assignment. > 3. When receiving COMPUTE_ASSIGNMENT error in ConsumerGroupHeartbeat API, > the consumer starts the assignment process. But what if somehow the member > didn't send out ConsumerGroupPrepareAssignment request, what would we do? I > think we'll wait for rebalance timeout and kick the member out of the > group, right? And then, what will the group coordinator do? I think it'll > select another member to be the leader and return COMPUTE_ASSIGNMENT error > in that member's heartbeat response, right? Maybe we should add that into > KIP. > The broker has a timer for this process, and if it did not receive the assignment (i.e. it is until the ConsumerGroupInstallAssignment is received; note that for some assignor they may not need to always refresh their information via ConsumerGroupPrepareAssignment upon computing the new assignment) in time, then the broker would pick another member and ask it to COMPUTE_ASSIGNMENT. But, we may not try to kick the member out of the group based on that timer, but only based on the heartbeat timer. I.e. let's say there's one member who keeps heartbeating but never want to compute the new assignment even when being asked, it would still be remained in the group. We can clarify this a bit more in the doc. > > Some typos: > 4. In "Assignment Process" section: > If the selected assignor does exist, the group coordinator will reject the > heartbeat with an UNSUPPORTED_ASSIGNOR error. > -> I think it should be "does not exist" > > Ack! > 5. In "JoinGroup Handling" section: > If the member has revoked all its partitions or the required partitions, > the member can transition to its next epoch. The current assignment become > the current assignment. > -> I think it should be "The target assignment becomes the current > assignment", right? > > Ack! > 6. In "ConsumerGroupDescribe API" section: > When the group coordinator handle a ConsumerGroupPrepareAssignmentRequest > request: > -> It should be "handle a ConsumerGroupDescribe request" > > Ack! > Again, thanks for the great proposal! > It has considered multiple edge cases. > Thank you. > > Luke > > On Wed, Jul 20, 2022 at 1: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
