Skip to content

Conversation

@ShivsundarR
Copy link
Collaborator

@ShivsundarR ShivsundarR commented Oct 30, 2025

What

  • Currently in ShareConsumerImpl, there is a chance we could be
    sending multiple ShareFetchEvents for a single poll().

  • This could happen when the ShareFetchBuffer is empty in the first
    wait and the pollTimer has not completed yet, then we would send
    multiple events.

  • Usually the ShareFetchBuffer would wait for a time until the
    pollTimeout, so we would not send multiple events, but the buffer wait
    time is also controlled by applicationEventHandler.maximumTimeToWait()
    which can return lower values (even 0) in some cases (especially during
    startup of heartbeat request manager).

  • If this happens, we will see multiple events sent, and this could even
    lead to multiple fetching from the broker (sort of a fetch and a
    pre-fetch) if the response for the previous ShareFetchEvent arrives
    before the next ShareFetchEvent was processed.

  • To avoid this, we have a check now which only sends one
    ShareFetchEvent per poll.

  • This was the reason a couple of tests were flaky in
    KafkaShareConsumerTest -
    https://issues.apache.org/jira/browse/KAFKA-18794. The PR fixes the
    flakiness, now the tests reliably pass locally.

Reviewers: Andrew Schofield [email protected]

@github-actions github-actions bot added triage PRs from the community consumer clients small Small PRs labels Oct 30, 2025
@ShivsundarR ShivsundarR added KIP-932 Queues for Kafka ci-approved and removed triage PRs from the community labels Oct 30, 2025
@github-actions github-actions bot removed the small Small PRs label Oct 31, 2025
Copy link
Member

@AndrewJSchofield AndrewJSchofield left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one trivial comment but we must maintain the standards :)

}

@Test
public void shouldSendOneShareFetchEventPerPoll() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: The tests should start with test.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have updated it now, thanks :))

@AndrewJSchofield AndrewJSchofield self-requested a review November 3, 2025 14:32
@AndrewJSchofield AndrewJSchofield merged commit c5ae7b4 into apache:trunk Nov 3, 2025
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants