I think it's reasonable for practical scenarios. Is it possible to turn off rack awareness in the clients in case the broker selector plugin is not compatible with rack-aware logic in the client?
-Artem On Wed, Nov 30, 2022 at 2:46 AM Rajini Sivaram <rajinisiva...@gmail.com> wrote: > Hi Artem, > > Understood your concern - brokers could use a replica selector that uses > some other client metadata other than rack id to decide the preferred read > replica, or just use the leader. In that case, consumers wouldn't really > benefit from aligning partition assignment on rack ids. So the question is > whether we should make the default consumer assignors use rack ids for > partition assignment or whether we should have different rack-aware > assignors that can be configured when brokers use rack-aware replica > selector. We had a similar discussion earlier in the thread (the KIP had > originally proposed new rack-aware assignors). We decided to update the > default assignors because: > 1) Consumers using fetch-from-follower automatically benefit from improved > locality, without having to update another config. > 2) Consumers configure rack id for improved locality, so aligning on > replica rack ids generally makes sense. > 3) We prioritize balanced assignment over locality in the consumer, so the > default assignors should work effectively regardless of broker's replica > selector. > > Does that make sense? > > > Thank you, > > Rajini > > > > On Tue, Nov 29, 2022 at 1:05 AM Artem Livshits > <alivsh...@confluent.io.invalid> wrote: > > > 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 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
