Skip to content
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

KAFKA-15078; KRaft leader replys with snapshot for offset 0 #13845

Merged

Conversation

jsancio
Copy link
Member

@jsancio jsancio commented Jun 12, 2023

If the follower has an empty log, fetches with offset 0, it is more efficient for the leader to reply with a snapshot id (redirect to FETCH_SNAPSHOT) than for the follower to continue fetching from the log segments.

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

If the follower has an empty log, fetches with offset 0, it is more
efficient for the leader to reply with a snapshot id (redirect to
FETCH_SNAPSHOT) than for the follower to continue fetching from the log
segments.
@jsancio jsancio added the kraft label Jun 12, 2023
}

@Test
public void testFetchRequestOffsetAtZero() throws Exception {
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 test fails against trunk.


Optional<OffsetAndEpoch> latestSnapshotId = log.latestSnapshotId();
final ValidOffsetAndEpoch validOffsetAndEpoch;
if (fetchOffset == 0 && latestSnapshotId.isPresent()) {
Copy link
Member

Choose a reason for hiding this comment

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

I have tried to figure out some other cases we prefer to read the snapshot, a common case is highWatermark - fetchOffset > count(snapshot record). Do you think it's possible to write count(snapshot record) in the snapshot header?

Copy link
Member Author

@jsancio jsancio Jun 20, 2023

Choose a reason for hiding this comment

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

Thanks @dengziming

We had a similar conversation in another PR: #13834 (comment)

In short, it is not clear to me that these improvements (implementation complexities) are a big win for the cluster metadata partition.

Copy link
Member

@dengziming dengziming left a comment

Choose a reason for hiding this comment

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

LGTM.

Copy link
Contributor

@mumrah mumrah left a comment

Choose a reason for hiding this comment

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

Thanks @jsancio. LGTM as well

@jsancio jsancio merged commit 3a246b1 into apache:trunk Jun 28, 2023
1 check failed
@jsancio jsancio deleted the kafka-15078-fetch-snapshot-from-empty-follower branch June 28, 2023 21:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants