Skip to content

KAFKA-20733: Fix fetchResponseWithUnexpectedPartitionIsIgnored passing for wrong reason with CONSUMER protocol#22651

Open
david-parkk wants to merge 4 commits into
apache:trunkfrom
david-parkk:KAFKA-20733
Open

KAFKA-20733: Fix fetchResponseWithUnexpectedPartitionIsIgnored passing for wrong reason with CONSUMER protocol#22651
david-parkk wants to merge 4 commits into
apache:trunkfrom
david-parkk:KAFKA-20733

Conversation

@david-parkk

@david-parkk david-parkk commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Summary

KafkaConsumerTest.fetchResponseWithUnexpectedPartitionIsIgnored was
annotated with @EnumSource(GroupProtocol.class), but the CONSUMER
protocol case passed for the wrong reason.

The CONSUMER protocol uses FetchRequestManager and relies on
ConsumerGroupHeartbeat for rebalancing. Since the test does not set up
heartbeat-based rebalancing, partition assignment never completes and 0
records are returned — not because FetchSessionHandler correctly
rejected the response.

Analysis

While writing the CONSUMER protocol test, we first attempted to use
AsyncKafkaConsumerTest as a reference. However, it was not suitable
because fetchCollector is replaced with a Mock, meaning
FetchSessionHandler's actual rejection logic is never exercised.

We also found that naively adding the CONSUMER case to
KafkaConsumerTest does not work either. The test uses
prepareResponse() for the Join and Sync group requests, but these
responses were never consumed — because the CONSUMER protocol does not
go through the classic Join/Sync rebalance flow. As a result, the fetch
phase was never reached. FetchRequestManagerTest is the correct
location for this test, as it directly runs FetchSessionHandler
without mocking.

Changes

  • KafkaConsumerTest: Restrict
    fetchResponseWithUnexpectedPartitionIsIgnored to CLASSIC protocol
    only, where the existing setup correctly exercises
    FetchSessionHandler's unexpected partition rejection logic.

  • FetchRequestManagerTest: Add
    testFetchResponseWithUnexpectedPartitionIsIgnored to properly cover
    the CONSUMER protocol path. The test verifies that when a
    FetchResponse contains a partition not included in the FetchRequest,
    FetchSessionHandler rejects the entire response and no records are
    returned to the consumer.

  • +KafkaConsumerTest: Rename
    fetchResponseWithUnexpectedPartitionIsIgnoredto
    testFetchResponseWithUnexpectedPartitionIsIgnored to align with the
    existing test naming convention in the class.

Reviewers: Lianet Magrans lmagrans@confluent.io

I'm happy to make any adjustments based on your feedback. Thank you to
the maintainers for taking the time to review this contribution!

@github-actions github-actions Bot added triage PRs from the community consumer tests Test fixes (including flaky tests) clients small Small PRs labels Jun 23, 2026
@lianetm lianetm added ci-approved and removed triage PRs from the community labels Jun 23, 2026

@lianetm lianetm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks! Just a nit

@ParameterizedTest
@EnumSource(GroupProtocol.class)
public void fetchResponseWithUnexpectedPartitionIsIgnored(GroupProtocol groupProtocol) {
@EnumSource(value = GroupProtocol.class, names = "CLASSIC")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

could you please add a comment here that CONSUMER is tested separately (just to avoid confusion, it's not that the asyncConsumer does not support this)

@david-parkk

david-parkk commented Jun 23, 2026

Copy link
Copy Markdown
Contributor Author

@lianetm Thanks for the review!
I've addressed your feedback — could you take another look when you get a chance?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci-approved clients consumer small Small PRs tests Test fixes (including flaky tests)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants