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