Hi Artem, Thanks for reviewing the KIP. The client doesn't rely on a particular replica assignment logic in the broker. Instead, it matches the actual rack assignment for partitions from the metadata with consumer racks. So there is no assumption in the client implementation about the use of RackAwareReplicaSelector in the broker. Did I misunderstand your concern?
Regards, Rajini On Tue, Nov 22, 2022 at 11:03 PM Artem Livshits <alivsh...@confluent.io.invalid> wrote: > Hi Rajini, > > Thank you for the KIP, the KIP looks good to match RackAwareReplicaSelector > behavior that is available out-of-box. Which should probably be good > enough in practice. > > From the design perspective, though, RackAwareReplicaSelector is just one > possible plugin, in theory the broker could use a plugin that leverages > networking information to get client locality or some other way, so it > seems like we're making an assumption about broker replica selection in the > default assignment implementation. So I wonder if we should use a separate > plugin that would be set when RackAwareReplicaSelector is set, rather than > assume broker behavior in the client implementation. > > -Artem > > On Wed, Nov 16, 2022 at 8:08 AM Jun Rao <j...@confluent.io.invalid> wrote: > > > Hi, David and Rajini, > > > > Thanks for the explanation. It makes sense to me now. > > > > Jun > > > > On Wed, Nov 16, 2022 at 1:44 AM Rajini Sivaram <rajinisiva...@gmail.com> > > wrote: > > > > > Thanks David, that was my understanding as well. > > > > > > Regards, > > > > > > Rajini > > > > > > On Wed, Nov 16, 2022 at 8:08 AM David Jacot > <dja...@confluent.io.invalid > > > > > > wrote: > > > > > > > Hi Jun, > > > > > > > > We don't need to bump any RPC requests. The subscription is > serialized > > > > (including its version) and included as bytes in the RPCs. > > > > > > > > Best, > > > > David > > > > > > > > On Tue, Nov 15, 2022 at 11:42 PM Jun Rao <j...@confluent.io.invalid> > > > wrote: > > > > > > > > > > Hi, Rajini, > > > > > > > > > > Thanks for the updated KIP. Just another minor comment. It would be > > > > useful > > > > > to list all RPC requests whose version needs to be bumped because > of > > > the > > > > > changes in ConsumerProtocolSubscription. > > > > > > > > > > Jun > > > > > > > > > > On Tue, Nov 15, 2022 at 3:45 AM Rajini Sivaram < > > > rajinisiva...@gmail.com> > > > > > wrote: > > > > > > > > > > > Hi David, > > > > > > > > > > > > Sorry, I was out of office and hence the delay in responding. > > Thanks > > > > for > > > > > > reviewing the KIP and answering Viktor's question (thanks for the > > > > review, > > > > > > Viktor). > > > > > > > > > > > > Responses below: > > > > > > 01) I was in two minds about adding new assignors, because as > you > > > > said, > > > > > > user experience is better if assignors used racks when available. > > But > > > > I was > > > > > > a bit concerned about changing the algorithm in existing > > applications > > > > which > > > > > > were already configuring `client.rack`. It felt less risky to add > > new > > > > > > assignor implementations instead. But we can retain existing > logic > > if > > > > a) > > > > > > rack information is not available and b) racks have all > partitions. > > > So > > > > the > > > > > > only case where logic will be different is when rack information > is > > > > > > available because consumers chose to use `client.rack` to benefit > > > from > > > > > > improved locality, but racks only have a subset of partitions. It > > > seems > > > > > > reasonable to make existing assignors rack-aware in this case to > > > > improve > > > > > > locality. I have updated the KIP. Will wait and see if there are > > any > > > > > > objections to this change. > > > > > > > > > > > > 02) Updated 1), so existing assignor classes will be used. > > > > > > > > > > > > 03) Updated the KIP to use version 3, thanks. > > > > > > > > > > > > If there are no concerns or further comments, I will start voting > > > later > > > > > > today. > > > > > > > > > > > > Thank you, > > > > > > > > > > > > Rajini > > > > > > > > > > > > > > > > > > On Fri, Nov 4, 2022 at 9:58 AM David Jacot > > > <dja...@confluent.io.invalid > > > > > > > > > > > wrote: > > > > > > > > > > > > > Hi Viktor, > > > > > > > > > > > > > > I can actually answer your question. KIP-848 already includes > > rack > > > > > > > awareness in the protocol. It is actually the other way around, > > > this > > > > > > > KIP takes the idea from KIP-848 to implement it in the current > > > > > > > protocol in order to realize the benefits sooner. The new > > protocol > > > > > > > will take a while to be implemented. > > > > > > > > > > > > > > Best, > > > > > > > David > > > > > > > > > > > > > > On Fri, Nov 4, 2022 at 10:55 AM David Jacot < > dja...@confluent.io > > > > > > > wrote: > > > > > > > > > > > > > > > > Hi Rajini, > > > > > > > > > > > > > > > > Thanks for the KIP. I have a few questions/comments: > > > > > > > > > > > > > > > > 01. If I understood correctly, the plan is to add new > assignors > > > > which > > > > > > > > are rack aware. Is this right? I wonder if it is a judicious > > > choice > > > > > > > > here. The main drawback is that clients must be configured > > > > correctly > > > > > > > > in order to get the benefits. From a user experience > > perspective, > > > > it > > > > > > > > would be much better if we would only require our users to > set > > > > > > > > client.rack. However, I understand the argument of keeping > the > > > > > > > > existing assignors as-is in order to limit the risk but it > also > > > > means > > > > > > > > that we will have to maintain multiple assignors with a > > somewhat > > > > > > > > similar core logic (within a rack). What do you think? > > > > > > > > > > > > > > > > 02. If we proceed with new rack-aware assignors, we should > > > mention > > > > > > > > their fully qualified names in the KIP as they will become > part > > > of > > > > our > > > > > > > > public interfaces. > > > > > > > > > > > > > > > > 03. KIP-792 has already introduced version 2 of the > > subscription. > > > > The > > > > > > > > KIP is accepted but the PR is not merged yet. This KIP should > > use > > > > > > > > version 3. > > > > > > > > > > > > > > > > Best, > > > > > > > > David > > > > > > > > > > > > > > > > On Thu, Nov 3, 2022 at 5:58 PM Viktor Somogyi-Vass > > > > > > > > <viktor.somo...@cloudera.com.invalid> wrote: > > > > > > > > > > > > > > > > > > Hi Rajini, > > > > > > > > > > > > > > > > > > If I understand correctly, the client.rack config would > stay > > > > > > supported > > > > > > > > > after KIP-848 but does it expand the scope of that KIP too > > with > > > > this > > > > > > > > > config? I mean that currently you propose > > > > > > ConsumerProtocolSubscription > > > > > > > to > > > > > > > > > be used but this protocol won't be available and we need to > > > > transfer > > > > > > > the > > > > > > > > > config to the coordinator via other means. Should this be > > added > > > > to > > > > > > > that KIP? > > > > > > > > > > > > > > > > > > Thanks, > > > > > > > > > Viktor > > > > > > > > > > > > > > > > > > On Wed, Nov 2, 2022 at 9:50 PM Rajini Sivaram < > > > > > > rajinisiva...@gmail.com > > > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > Hi Jun, > > > > > > > > > > > > > > > > > > > > Thank you for the review. Yes, we should add rack id to > > > > > > > Subscription, had > > > > > > > > > > missed that part. Updated the KIP, thank you for pointing > > > that > > > > out! > > > > > > > > > > > > > > > > > > > > Regards, > > > > > > > > > > > > > > > > > > > > Rajini > > > > > > > > > > > > > > > > > > > > On Wed, Nov 2, 2022 at 7:06 PM Jun Rao > > > > <j...@confluent.io.invalid> > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > Hi, Rajini, > > > > > > > > > > > > > > > > > > > > > > Thanks for the KIP. Just one comment. > > > > > > > > > > > > > > > > > > > > > > Should we add rackId to GroupSubscription.Subscription > > for > > > > each > > > > > > > member? > > > > > > > > > > > > > > > > > > > > > > Thanks, > > > > > > > > > > > > > > > > > > > > > > Jun > > > > > > > > > > > > > > > > > > > > > > On Wed, Nov 2, 2022 at 4:57 AM Rajini Sivaram < > > > > > > > rajinisiva...@gmail.com> > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > Hi all, > > > > > > > > > > > > > > > > > > > > > > > > I have submitted KIP-881 to implement rack-aware > > > partition > > > > > > > assignment > > > > > > > > > > for > > > > > > > > > > > > consumers: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-881%3A+Rack-aware+Partition+Assignment+for+Kafka+Consumers > > > > > > > > > > > > . > > > > > > > > > > > > It adds rack id to the consumer group protocol to > > > propagate > > > > > > rack > > > > > > > > > > > > information so that rack-aware assignors can be added > > to > > > > > > benefit > > > > > > > from > > > > > > > > > > > > locality. > > > > > > > > > > > > > > > > > > > > > > > > Feedback and suggestions are welcome! > > > > > > > > > > > > > > > > > > > > > > > > Thank you, > > > > > > > > > > > > > > > > > > > > > > > > Rajini > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
