KAFKA-20585 : Share Fetch latency metric incorrectly updated during ShareAcknowledgeResponse by muralibasani · Pull Request #22296 · apache/kafka

muralibasani · 2026-05-15T20:37:27Z

Ref : https://issues.apache.org/jira/browse/KAFKA-20585

fetch latency metric is recorded twice. In handleShareFetchSuccess and
handleShareAcknowledgeSuccess.

Removing from handleShareAcknowledgeSuccess

Reviewers: Shivsundar R shr@confluent.io, Apoorv Mittal apoorvmittal10@gmail.com

nileshkumar3

Thanks for PR, +1 on the fix.

It looks like there is no dedicated ack latency sensor in ShareFetchMetricsManager today. If ack latency is useful to track, this may be worth a follow-up JIRA rather than reusing fetch metrics.

Also, consider adding a test asserting that fetch-rate/fetch-total do not change after a successful ShareAcknowledge response, to guard against regression.

chia7712 · 2026-05-17T03:02:00Z

                metadata.updatePartitionLeadership(partitionsWithUpdatedLeaderInfo, leaderNodes);
            }

-            if (acknowledgeRequestState.isProcessed) {


The metrics are being recorded at the end of the response handler, so we would be missing to update the share-fetch-rate and share-fetch-total metrics for responses with top level errors. This is the same pattern in FetchManager as well where we are missing these responses with top level errors. Not sure if this was intended or we should account for these as well. Lianet Magrans could you help take a look at the regular consumer/share-consumer and see what we should do here?

Is this addressed?

@chia7712 thank you for the review.
I pushed a commit to address this. I also commented in the ticket about this change, if that looks ok.

@muralibasani For my understanding, if there is only ShareAcknowledge request then will we still record latency? I can see code only in handleShareFetchSuccess now.

@apoorvmittal10 sorry for delayed reply.
And that's correct. With this change, latency is recorded only in handleShareFetchSuccess, so a ShareAcknowledge-only round trip no longer records latncy. I do not see any dedicated acknowledge-sensor now.

muralibasani · 2026-05-17T08:20:12Z

Thanks for PR, +1 on the fix.

It looks like there is no dedicated ack latency sensor in ShareFetchMetricsManager today. If ack latency is useful to track, this may be worth a follow-up JIRA rather than reusing fetch metrics.

Also, consider adding a test asserting that fetch-rate/fetch-total do not change after a successful ShareAcknowledge response, to guard against regression.

@nileshkumar3 thanks for the review. IMO this was copy pasted by mistake and we ended up with this duplicate in ack method. We might have other metrics in these round trip situations. If you still feel it's worth adding, I will.

muralibasani · 2026-05-17T08:20:26Z

Thanks for PR, +1 on the fix.

It looks like there is no dedicated ack latency sensor in ShareFetchMetricsManager today. If ack latency is useful to track, this may be worth a follow-up JIRA rather than reusing fetch metrics.

Also, consider adding a test asserting that fetch-rate/fetch-total do not change after a successful ShareAcknowledge response, to guard against regression.

Reg the follow-up jira, I think it makes sense. acknowledge-latency wdyt @chia7712 ?

nileshkumar3 · 2026-05-17T14:25:14Z

            final ShareFetchResponse response = (ShareFetchResponse) resp.responseBody();
            final ShareSessionHandler handler = sessionHandler(fetchTarget.id());

+            metricsManager.recordLatency(resp.destination(), resp.requestLatencyMs());


should we move it to finally ?

moving it to finally will record the metric even if above lines 817 or 818 fail (while getting response and handler), and we don't want that right.

Given the JIRA intent of capturing top-level errors too, IMO finally is a cleaner fit. Curious what @chia7712 / @apoorvmittal10 think.

nileshkumar3 · 2026-05-17T14:32:50Z

Thanks for PR, +1 on the fix.
It looks like there is no dedicated ack latency sensor in ShareFetchMetricsManager today. If ack latency is useful to track, this may be worth a follow-up JIRA rather than reusing fetch metrics.
Also, consider adding a test asserting that fetch-rate/fetch-total do not change after a successful ShareAcknowledge response, to guard against regression.

@nileshkumar3 thanks for the review. IMO this was copy pasted by mistake and we ended up with this duplicate in ack method. We might have other metrics in these round trip situations. If you still feel it's worth adding, I will.

IMO, adding a regression test for this issue worths, other metrics may be a follow up.

nileshkumar3 · 2026-05-17T14:27:15Z

            final FetchResponse response = (FetchResponse) resp.responseBody();
            final FetchSessionHandler handler = sessionHandler(fetchTarget.id());

+            metricsManager.recordLatency(resp.destination(), resp.requestLatencyMs());


moving it to finally will record the metric even if above lines 155 or 156 fail (while getting response and handler), and we don't want that right., same as above

muralibasani · 2026-05-24T19:05:20Z

        metrics = new Metrics(metricConfig, time);
        shareFetchMetricsRegistry = new ShareFetchMetricsRegistry(metricConfig.tags().keySet(), "consumer-share" + groupId);
-        metricsManager = new ShareFetchMetricsManager(metrics, shareFetchMetricsRegistry);
+        metricsManager = spy(new ShareFetchMetricsManager(metrics, shareFetchMetricsRegistry));


Using spy here because the existing tests need the real metric registry to work

lianetm · 2026-05-25T14:49:49Z

            final FetchResponse response = (FetchResponse) resp.responseBody();
            final FetchSessionHandler handler = sessionHandler(fetchTarget.id());

+            metricsManager.recordLatency(resp.destination(), resp.requestLatencyMs());


this line should fix the gap for the Async and Classic consumer I expect.

We would just need to add tests to validate it (same as the one added here for the share consumer, but using an error like FETCH_SESSION_TOPIC_ID_ERROR maybe), added to FetcherTest for the classic, and in FetchRequestManagerTest for the async.

Thanks @lianetm for the review.
Added tests for classic(FetcherTest) and async(FetchRequestManagerTest)

ShivsundarR

Thanks for the PR @muralibasani , LGTM!

muralibasani · 2026-06-04T14:54:19Z

@lianetm would you be able to take another look ? thanks.

muralibasani · 2026-06-17T08:46:23Z

@chia7712 can you pls take a look?

apoorvmittal10 · 2026-06-29T15:55:43Z

@muralibasani I will read it, sorry for the delay.

apoorvmittal10

Thanks for the changes, a question for my understanding.

apoorvmittal10 · 2026-06-29T21:59:06Z

                metadata.updatePartitionLeadership(partitionsWithUpdatedLeaderInfo, leaderNodes);
            }

-            if (acknowledgeRequestState.isProcessed) {


@muralibasani For my understanding, if there is only ShareAcknowledge request then will we still record latency? I can see code only in handleShareFetchSuccess now.

…eResponse

muralibasani · 2026-07-10T16:11:11Z

Thanks for the changes, a question for my understanding.

@apoorvmittal10 thanks for the review.

github-actions Bot added triage PRs from the community consumer clients small Small PRs labels May 15, 2026

chia7712 added the ci-approved label May 15, 2026

nileshkumar3 reviewed May 16, 2026

View reviewed changes

smjn requested a review from apoorvmittal10 May 16, 2026 20:16

chia7712 reviewed May 17, 2026

View reviewed changes

github-actions Bot removed the triage PRs from the community label May 17, 2026

nileshkumar3 reviewed May 17, 2026

View reviewed changes

muralibasani commented May 24, 2026

View reviewed changes

muralibasani force-pushed the KAFKA-20585 branch 2 times, most recently from c82160e to 50f8e95 Compare May 24, 2026 19:21

lianetm reviewed May 25, 2026

View reviewed changes

ShivsundarR approved these changes May 26, 2026

View reviewed changes

apoorvmittal10 reviewed Jun 29, 2026

View reviewed changes

muralibasani added 2 commits July 10, 2026 18:05

Share Fetch latency metric incorrectly updated during ShareAcknowledg…

fbabc17

…eResponse

Adding tests for async and classic consumer

0c09182

muralibasani force-pushed the KAFKA-20585 branch from 8adf4cd to 0c09182 Compare July 10, 2026 16:05

Uh oh!

Conversation

muralibasani commented May 15, 2026 • edited by github-actions Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nileshkumar3 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

muralibasani commented May 17, 2026

Uh oh!

muralibasani commented May 17, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nileshkumar3 May 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nileshkumar3 commented May 17, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ShivsundarR left a comment

Choose a reason for hiding this comment

Uh oh!

muralibasani commented Jun 4, 2026

Uh oh!

muralibasani commented Jun 17, 2026

Uh oh!

apoorvmittal10 commented Jun 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

apoorvmittal10 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

muralibasani commented Jul 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

muralibasani commented May 15, 2026 •

edited by github-actions Bot

Loading

nileshkumar3 May 17, 2026 •

edited

Loading

apoorvmittal10 commented Jun 29, 2026 •

edited

Loading