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 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
