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