I'm thinking of a case where the broker uses a plugin that does not use rack ids to determine client locality, in that case the consumer might arrive at the wrong decision based on rack ids.
-Artem On Wed, Nov 23, 2022 at 3:54 AM Rajini Sivaram <rajinisiva...@gmail.com> wrote: > 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 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
