Jun

Thanks for the feedback.

Numbers to follow.

If we don't plan to
> bump up the FetchResponse version, we could just remove the reference to
> version 16.

Fixed.

On Thu, Jul 20, 2023 at 1:28 AM Jun Rao <j...@confluent.io.invalid> wrote:

> Hi, Mayank,
>
> Thanks for the KIP. I agree with others that it would be useful to see the
> performance results. Otherwise, just a minor comment. If we don't plan to
> bump up the FetchResponse version, we could just remove the reference to
> version 16.
>
> Jun
>
> On Wed, Jul 19, 2023 at 2:31 PM Mayank Shekhar Narula <
> mayanks.nar...@gmail.com> wrote:
>
> > Luke
> >
> > Thanks for the interest in the KIP.
> >
> > But what if the consumer was fetching from the follower?
> >
> > We already include `PreferredReadReplica` in the fetch response.
> > > Should we put the node info of PreferredReadReplica under this case,
> > > instead of the leader's info?
> > >
> >
> > PreferredReadReplica is the decided on the leader. Looking at the Java
> > client code, AbstractFetch::selectReadReplica, first fetch request goes
> to
> > Leader of the partition -> Sends back PreferredReadReplica -> Next fetch
> > uses PreferredReadReplica. So as long as leader is available,
> > PreferredReadReplica would be found in subsequent fetches.
> >
> > Also, under this case, should we include the leader's info in the
> response?
> >
> >
> > In this case, I think the follower would fail the fetch if it knows a
> > different leader. If the follower knows a newer leader, it would return
> new
> > leader information in the response, for the client to act on.
> >
> >
> > Will we include the leader/node info in the response when having
> > > `UNKNOWN_LEADER_EPOCH` error?
> >
> >
> > My understanding is UNKNOWN_LEADER_EPOCH when a request from a client
> has a
> > newer epoch than the broker. So the client is already up to date on new
> > leader information, it's the broker that has the catching up to do. I
> think
> > there might be some optimisations to make sure the broker refreshes its
> > metadata quickly, so it can quickly recover to handle requests that
> > previously returned UNKNOWN_LEADER_EPOCH. But this work is outside the
> > scope of this KIP, as for now this KIP focusses on client-side
> > optimisations.
> >
> > Mayank
> >
> > On Tue, Jul 18, 2023 at 8:51 AM Luke Chen <show...@gmail.com> wrote:
> >
> > > Hi Mayank,
> > >
> > > Thanks for the KIP!
> > >
> > > Some questions:
> > > 1. I can see most of the cases we only care about consumer fetch from
> the
> > > leader.
> > > But what if the consumer was fetching from the follower?
> > > We already include `PreferredReadReplica` in the fetch response.
> > > Should we put the node info of PreferredReadReplica under this case,
> > > instead of the leader's info?
> > > Also, under this case, should we include the leader's info in the
> > response?
> > >
> > > 2. Will we include the leader/node info in the response when having
> > > `UNKNOWN_LEADER_EPOCH` error?
> > > I think it's fine we ignore the `UNKNOWN_LEADER_EPOCH` error since when
> > > this happens, the node might have some error which should refresh the
> > > metadata. On the other hand, it might also be good if we can heal the
> > node
> > > soon to do produce/consume works.
> > >
> > >
> > > Thank you.
> > > Luke
> > >
> > > On Tue, Jul 18, 2023 at 2:00 AM Philip Nee <philip...@gmail.com>
> wrote:
> > >
> > > > Hey Mayank:
> > > >
> > > > For #1: I think fetch and produce behave a bit differently on
> metadata.
> > > > Maybe it is worth highlighting the changes for each client in detail.
> > In
> > > > producer did you mean by the metadata timeout before sending out
> > produce
> > > > requests? For consumer: I think for fetches it requires user to retry
> > if
> > > > the position does not exist on the leader. I don't have the detail on
> > top
> > > > of my head, but I think we should lay out these behavioral changes.
> > > >
> > > > For #3: Thanks for the clarification.
> > > >
> > > > On Mon, Jul 17, 2023 at 10:39 AM Mayank Shekhar Narula <
> > > > mayanks.nar...@gmail.com> wrote:
> > > >
> > > > > Philip
> > > > >
> > > > > 1. Good call out about "poll" behaviour, my understanding is the
> > same.
> > > I
> > > > am
> > > > > assuming it's about the motivation of the KIP. There with async, my
> > > > > intention was to convey that the client doesn't wait for the
> > > > > metadata-refresh before a subsequent retry of the produce or fetch
> > > > request
> > > > > that failed due to stale metadata(i.e. going to an old leader). The
> > > only
> > > > > wait client has is the configured retry-delay.
> > > > >
> > > > > 2. Yes, in theory other APIs could benefit from this too. But that
> is
> > > > > outside of the scope of the KIP.
> > > > >
> > > > > 3. Do you mean the response for the Metadata RPC? I think brokers
> > > always
> > > > > have a view of the cluster, although it can be stale,it would
> always
> > > > return
> > > > > a leader(whether old or new).
> > > > >
> > > > > Mayank
> > > > >
> > > > > On Fri, Jul 14, 2023 at 8:53 PM Philip Nee <philip...@gmail.com>
> > > wrote:
> > > > >
> > > > > > Hey Mayank,
> > > > > >
> > > > > > Thanks for the KIP. I think this is a great proposal, and I'm in
> > > favor
> > > > > > of this idea.  A few comments:
> > > > > >
> > > > > > 1. Claiming metadata refresh is done asynchronously is
> misleading.
> > > The
> > > > > > metadata refresh requires Network Client to be physically polled,
> > > which
> > > > > is
> > > > > > done in a separate thread in Producer and Admin Client (IIRC!)
> but
> > > not
> > > > > > Consumer.
> > > > > > 2. There are other API calls that might result in
> > > > NOT_LEADER_OR_FOLLOWER
> > > > > > response, but it seems like this KIP only wants to update on
> fetch
> > > and
> > > > > > produce. Do we want to make the leader information available for
> > > other
> > > > > API
> > > > > > calls?
> > > > > > 3. Do you know what would happen during a leader election? I'm
> not
> > > sure
> > > > > > about this process and I wonder if the current metadata response
> > uses
> > > > the
> > > > > > old leader or null as the leader isn't readily available yet.
> > > > > >
> > > > > > Thanks,
> > > > > > P
> > > > > >
> > > > > > On Fri, Jul 14, 2023 at 11:30 AM Kirk True <k...@kirktrue.pro>
> > > wrote:
> > > > > >
> > > > > > > Hi Mayank,
> > > > > > >
> > > > > > > > On Jul 14, 2023, at 11:25 AM, Mayank Shekhar Narula <
> > > > > > > mayanks.nar...@gmail.com> wrote:
> > > > > > > >
> > > > > > > > Kirk
> > > > > > > >
> > > > > > > >
> > > > > > > >> Is the requested restructuring of the response “simply” to
> > > > preserve
> > > > > > > bytes,
> > > > > > > >> or is it possible that the fetch response could/should/would
> > > > return
> > > > > > > >> leadership changes for partitions that we’re specifically
> > > > requested?
> > > > > > > >>
> > > > > > > >
> > > > > > > > Moving endpoints to top-level fields would preserve bytes,
> > > > otherwise
> > > > > > the
> > > > > > > > endpoint-information would be duplicated if included with the
> > > > > > > > partition-data in the response. Right now, the top-level
> field
> > > will
> > > > > > only
> > > > > > > be
> > > > > > > > set in case leader changes for any requested partitions. But
> it
> > > can
> > > > > be
> > > > > > > > re-used in the future, for which Jose has a use-case in mind
> > > shared
> > > > > up
> > > > > > in
> > > > > > > > the thread. KIP is now upto date with endpoint info being at
> > > > > top-level.
> > > > > > >
> > > > > > >
> > > > > > > I didn’t catch before that there was a separate section for the
> > > full
> > > > > node
> > > > > > > information, not just the ID and epoch.
> > > > > > >
> > > > > > > Thanks!
> > > > > > >
> > > > > > > >>> 3. In the future, I may use this information in the
> > > > KRaft/Metadata
> > > > > > > >>> implementation of FETCH. In that implementation not all of
> > the
> > > > > > > >>> replicas are brokers.
> > > > > > > >>
> > > > > > > >> Side point: any references to the change you’re referring
> to?
> > > The
> > > > > idea
> > > > > > > of
> > > > > > > >> non-brokers serving as replicas is blowing my mind a bit :)
> > > > > > > >>
> > > > > > > >>
> > > > > > > > Jose, I missed this as well, would love to know more about
> > > > non-broker
> > > > > > > > serving as replica!
> > > > > > > > --
> > > > > > > > Regards,
> > > > > > > > Mayank Shekhar Narula
> > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > > >
> > > > > --
> > > > > Regards,
> > > > > Mayank Shekhar Narula
> > > > >
> > > >
> > >
> >
> >
> > --
> > Regards,
> > Mayank Shekhar Narula
> >
>


-- 
Regards,
Mayank Shekhar Narula

Reply via email to