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