Skip to content

KAFKA-19154; Offset Fetch API should return INVALID_OFFSET if requested topic id does not match persisted one #19744

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: trunk
Choose a base branch
from

Conversation

dajac
Copy link
Member

@dajac dajac commented May 16, 2025

This patch updates the OffsetFetch API to ensure that a committed offset
is returned iff the requested topic id matches the persisted one; the
invalid offset is returned otherwise.

@dajac dajac added the KIP-848 The Next Generation of the Consumer Rebalance Protocol label May 16, 2025
@github-actions github-actions bot added the core Kafka Broker label May 16, 2025
@@ -906,7 +907,7 @@ public OffsetFetchResponseData.OffsetFetchResponseGroup fetchOffsets(
.setCommittedOffset(INVALID_OFFSET)
.setCommittedLeaderEpoch(-1)
.setMetadata(""));
} else if (offsetAndMetadata == null) {
} else if (offsetAndMetadata == null || isMismatchedTopicId(offsetAndMetadata.topicId, topic.topicId())) {
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the core change.

Comment on lines +533 to +540
// There are two ways to ensure that committed of recreated topics are not returned.
// 1) When a topic is deleted, GroupCoordinatorService#onPartitionsDeleted is called to
// delete all its committed offsets.
// 2) Since version 10 of the OffsetCommit API, the topic id is stored alongside the
// committed offset. When it is queried, it is only returned iff the topic id of
// committed offset matches the requested one.
// The test tests both conditions but not in a deterministic way as they race
// against each others.
Copy link
Member Author

Choose a reason for hiding this comment

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

I am not happy with this but I don't have a good idea to make it better. I was thinking about introducing an internal config/parameter to disable 1) in order to test 2) but it feels weird to have this in production code. Thoughts?

I have tested it manually by commenting out 1).

@dajac dajac requested a review from lianetm May 16, 2025 18:48
@dajac
Copy link
Member Author

dajac commented May 16, 2025

@lianetm @squah-confluent @dongnuo123 Could you please review it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Kafka Broker KIP-848 The Next Generation of the Consumer Rebalance Protocol
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant