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

Reply via email to