KAFKA-20585 : Share Fetch latency metric incorrectly updated during ShareAcknowledgeResponse

[muralibasani]

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

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

• Removing from handleShareAcknowledgeSuccess

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

[muralibasani]

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]

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]

should we move it to finally ?

[muralibasani]

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.

[nileshkumar3]

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

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

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

same here

[muralibasani]

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]

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

[lianetm]

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.