Hi Artem, Yes, clients can reset their `client.rack` config if the broker doesn't use a rack-aware selector. This config is only useful for improving locality with a rack-aware selector.
On Wed, Nov 30, 2022 at 10:05 PM Artem Livshits <alivsh...@confluent.io.invalid> wrote: > 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 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
