Hi David
01 Same as Java Client's existing behaviour of requesting a full expedited
metadata-refresh done asynchronously, for error NOT_LEADER OR FENCED_EPOCH,
the KIP proposes to continue doing so. As this would help prevent similar
errors on future requests to other partitions affected by
Hi Mayank,
Thanks again for the KIP and thanks for adding the new analysis. Overall, I
am fine with it. I have a few minor comments.
01. If I understand correctly, the client will still request a metadata
update even when it gets the new leader if the produce response or the
fetch response. Is
Adding to Crispin. The new micro-benchmark shows improvements of 88% in
p99.9 with the KIP changes Vs baseline(& rejected alternate). Its
hypothesised improvements are seen as KIP avoids a full metadata refresh(Vs
baseline/rejected alternate), and the full metadata refresh can be slow due
to
Hi,
I've updated the KIP with benchmark results focusing more directly on the
redirect case, please review and +1 in the voting thread if convinced.
Thank you,
Crispin
On Wed, Jul 26, 2023 at 11:13 PM Luke Chen wrote:
> Thanks for adding the benchmark results, Crispin!
> IMO, 2~5% performance
Thanks for adding the benchmark results, Crispin!
IMO, 2~5% performance improvement is small, but given the change is small,
cost is also small (only append endpoint info when NOT_LEADER_OR_FOLLOWER..
etc error), I think it is worth doing it.
Thank you.
Luke
On Wed, Jul 26, 2023 at 12:33 AM
Thanks Crispin!
Ismael
On Mon, Jul 24, 2023 at 8:16 PM Crispin Bernier
wrote:
> I updated the wiki to include both results along with their average.
>
> Thank you,
> Crispin
>
> On Mon, Jul 24, 2023 at 10:58 AM Ismael Juma wrote:
>
> > Hi Crispin,
> >
> > One additional question, the wiki
I updated the wiki to include both results along with their average.
Thank you,
Crispin
On Mon, Jul 24, 2023 at 10:58 AM Ismael Juma wrote:
> Hi Crispin,
>
> One additional question, the wiki says "The results are averaged over 2
> runs.". Can you please provide some measure of variance in the
David,
We never backport new features to old releases. This new feature will be
> only available from 3.6 (or 3.7) onwards for both client and server.
Good to know. I think that makes the argument for bumping the version even
stronger.
On Mon, Jul 24, 2023 at 5:01 PM David Jacot
wrote:
> Hi
Hi Mayank,
We never backport new features to old releases. This new feature will be
only available from 3.6 (or 3.7) onwards for both client and server.
Best,
David
On Mon, Jul 24, 2023 at 5:20 PM Mayank Shekhar Narula <
mayanks.nar...@gmail.com> wrote:
> Thanks Jose/David/Ismael for your
Hi Mayank,
On Mon, Jul 24, 2023 at 8:21 AM Mayank Shekhar Narula
wrote:
>
> Thanks Jose/David/Ismael for your inputs.
>
> Not bumping the version, would require both broker & client to backport
> changes. Especially for FetchResponse, as backporting would have to be done
> all the way back to
Thanks Jose/David/Ismael for your inputs.
Not bumping the version, would require both broker & client to backport
changes. Especially for FetchResponse, as backporting would have to be done
all the way back to 3.1, so this effort isn't trivial, and was originally
underestimated.
Considering
Hi Crispin,
One additional question, the wiki says "The results are averaged over 2
runs.". Can you please provide some measure of variance in the
distribution, i.e. were both results similar to each other for both cases?
Ismael
On Fri, Jul 21, 2023 at 11:31 AM Ismael Juma wrote:
> Thanks for
On Mon, Jul 24, 2023 at 3:32 PM David Jacot
wrote:
> 01. Hum... I understand your reasoning. I think that this is mainly
> beneficial for clients lagging behind in terms of supported versions.
> However, it is the opposite for the java client which is up to date.
> Personally, I would rather
Hi Mayank,
01. Hum... I understand your reasoning. I think that this is mainly
beneficial for clients lagging behind in terms of supported versions.
However, it is the opposite for the java client which is up to date.
Personally, I would rather prefer to bump both versions and to add the
tagged
Hey Mayank,
It is probably binary compatible to have the NodeEndponts fielld at
taggedVersion 12+ but I think it is misleading as a code reviewer. The
Java Kafka client at version 12 will never be able to handle those
fields. Or are you planning to backport these improvements to those
clients and
The downsides of bumping the version is that clients have to have all
the latest features implemented before being able to benefit from this
performance improvement.
One of the benefits of using a tagged field is to make the field
available to previous versions too.
Choosing a minimum value
David
03. Fixed as well, to remove ignorable. In MetadataResponse, Rack came a
version later, hence was marked ignorable.
Thanks.
On Fri, Jul 21, 2023 at 1:38 PM Mayank Shekhar Narula <
mayanks.nar...@gmail.com> wrote:
> Hi David
>
> 01. My reasoning noted in the KIP is that CurrentLeader was
Hi David
01. My reasoning noted in the KIP is that CurrentLeader was first added in
version 12, so 12 is the least version where clients could get these
optimisations. So any client can now choose to implement this with version
12 of the protocol itself. If the version is bumped to X(say 16),
Thanks for the update Crispin - very helpful to have actual performance
data. 2-5% for the default configuration is a bit on the low side for this
kind of proposal.
Ismael
On Thu, Jul 20, 2023 at 11:33 PM Crispin Bernier
wrote:
> Benchmark numbers have been posted on the KIP, please review.
>
Hi Mayank,
Thanks for the KIP. This is an interesting idea that I have been thinking
about for a long time so I am happy to see it. The gain is smaller than I
expected but still worth it in my opinion.
01. In the FetchResponse, what's the reason for using version `12+` for the
new tagged field
Benchmark numbers have been posted on the KIP, please review.
On 2023/07/20 13:03:00 Mayank Shekhar Narula wrote:
> 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.
>
>
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 wrote:
> Hi, Mayank,
>
> Thanks for the KIP. I agree with others that it would be
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
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
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
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
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
Kirk
1. For the client, it doesn't matter whether the server is KRaft or ZK.
Client behaviour will be simply driven by the protocol-changes proposed for
the FetchResponse & ProduceResponse. On the server side, there will be
differences on how a new leader is discovered depending on whether it's
Hi Krik,
On Fri, Jul 14, 2023 at 10:59 AM Kirk True wrote:
> 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?
Both. My
Emanuele
I agree with this. That's why i quoted below -
> I wonder if non-Kafka clients might benefit from not bumping the
> version. If versions are bumped, say for FetchResponse to 16, I believe
> that client would have to support all versions until 16 to fully
> utilise this feature. Whereas,
The downsides of bumping the version is that clients have to have all
the latest features implemented before being able to benefit from this
performance improvement.
One of the benefits of using a tagged field is to make the field
available to previous versions too.
Choosing a minimum value
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
Hi Mayank,
> On Jul 14, 2023, at 11:25 AM, Mayank Shekhar Narula
> 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
Hi Andrew,
Good point.
Sorry to be dim bulb, but I’m still not sure I understand the downsides of
bumping the version. The broker and all client implementations would have to
change to fully support this feature anyway, but down version clients are
handled by brokers already.
Thanks,
Kirk
>
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
Hi Mayank,
Thanks for the KIP!
Questions:
1. From the standpoint of the client, does it matter if the cluster is running
in KRaft mode vs. Zookeeper? Will the behavior somehow be subtlety different
given that metadata propagation is handled differently between the two?
2. Is there anything
Jose,
Using the term "node" makes sense since it follows convention, and then
"NodeEndpoints" can be reused in future. Update the KIP with protocol
message changes.
On Fri, Jul 14, 2023 at 5:53 PM José Armando García Sancio
wrote:
> Hi Mayank,
>
> On Thu, Jul 13, 2023 at 10:03 AM Mayank
Hi Mayank/José,
> On Jul 13, 2023, at 8:20 AM, José Armando García Sancio
> wrote:
>
> Hi Mayank, thanks for the KIP. I look forward to this improvement for
> new clients.
>
> Some comments below.
>
> On Thu, Jul 13, 2023 at 7:15 AM Mayank Shekhar Narula
> wrote:
>> Following KIP is up for
Hi Ismael
Right now, effort is to get baseline Vs KIP-proposed changes #s.
After that will look at the alternative #s.
On Fri, Jul 14, 2023 at 12:38 AM Ismael Juma wrote:
> Hi Mayank,
>
> See my answer below.
>
> On Thu, Jul 13, 2023 at 10:24 AM Mayank Shekhar Narula <
>
Hi Mayank,
On Thu, Jul 13, 2023 at 10:03 AM Mayank Shekhar Narula
wrote:
> 3. If I understood this correctly, certain replicas "aren't" brokers, what
> are they then?
In a Kafka KRaft cluster they can be either brokers, controllers or
both. The term we use is node. A Kafka node can be either a
Hi Mayank,
See my answer below.
On Thu, Jul 13, 2023 at 10:24 AM Mayank Shekhar Narula <
mayanks.nar...@gmail.com> wrote:
> re. 2 On some busy clusters a single metadata call has been observed to
> take order of ~100 milliseconds(I think it's mentioned somewhere in this
> motivation). So
Hi Mayank,
If we bump the version, the broker can tell whether it’s worth providing the
leader
endpoint information to the client when the leader has changed. That’s my
reasoning.
Thanks,
Andrew
> On 13 Jul 2023, at 18:02, Mayank Shekhar Narula
> wrote:
>
> Thanks both for looking into
Ismael
Thanks for the feedback. 1/3 are in the pipeline.
re. 2 On some busy clusters a single metadata call has been observed to
take order of ~100 milliseconds(I think it's mentioned somewhere in this
motivation). So retrying immediately on the Produce path won't make sense,
as metadata would
Thanks both for looking into this.
Jose,
1/2 & 4(changes for PRODUCE) & 5 makes sense, will follow
3. If I understood this correctly, certain replicas "aren't" brokers, what
are they then?
Also how about replacing "Replica" with "Leader", this is more readable on
the client. so, how about
Thanks for the KIP. A couple of high level points and a more specific one:
1. Given the motivation, the performance results are essential for proper
evaluation, so I am looking forward to those.
2. The reasoning given for the rejected alternative seems weak to me. It
would be useful to have
Hi José,
Thanks. Sounds good.
Andrew
> On 13 Jul 2023, at 16:45, José Armando García Sancio
> wrote:
>
> Hi Andrew,
>
> On Thu, Jul 13, 2023 at 8:35 AM Andrew Schofield
> wrote:
>> I have a question about José’s comment (2). I can see that it’s possible for
>> multiple
>> partitions to
Hi Andrew,
On Thu, Jul 13, 2023 at 8:35 AM Andrew Schofield
wrote:
> I have a question about José’s comment (2). I can see that it’s possible for
> multiple
> partitions to change leadership to the same broker/node and it’s wasteful to
> repeat
> all of the connection information for each
Hi,
Thanks for the KIP, Mayank.
I have a question about José’s comment (2). I can see that it’s possible for
multiple
partitions to change leadership to the same broker/node and it’s wasteful to
repeat
all of the connection information for each topic-partition. But, I think it’s
important to
Hi Mayank, thanks for the KIP. I look forward to this improvement for
new clients.
Some comments below.
On Thu, Jul 13, 2023 at 7:15 AM Mayank Shekhar Narula
wrote:
> Following KIP is up for discussion. Thanks for your feedback
Regarding the FETCH response changes:
1. Tagged field 2 already
49 matches
Mail list logo
