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-7568; Return leader epoch in ListOffsets response #5855

Merged
merged 2 commits into from
Nov 1, 2018

Conversation

hachikuji
Copy link

As part of KIP-320, the ListOffsets API should return the leader epoch of any fetched offset. We either get this epoch from the log itself for a timestamp query or from the epoch cache if we are searching the earliest or latest offset in the log. When handling queries for the latest offset, we have elected to choose the current leader epoch, which is consistent with other handling (e.g. OffsetsForTimes).

Committer Checklist (excluded from commit message)

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

@ijuma
Copy link
Contributor

ijuma commented Oct 30, 2018

Is this needed for 2.1?

@hachikuji
Copy link
Author

hachikuji commented Oct 30, 2018

@ijuma Nothing is depending on it yet, so I think it is not strictly needed. Nevertheless, I was considering bugging @lindong28 about it (late though it is) since the changes are straightforward and it is a little annoying to have an incomplete version bump.

@ijuma
Copy link
Contributor

ijuma commented Oct 30, 2018

RC0 has already gone out, so unless it's a blocker, we should not include it.

@hachikuji
Copy link
Author

@ijuma I am fine with it going to trunk only.

Copy link
Contributor

@junrao junrao left a comment

Choose a reason for hiding this comment

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

@hachikuji : Thanks for the patch. Looks good overall. Just one comment below.

@@ -1279,9 +1288,11 @@ class Log(@volatile var dir: File,
val segmentsCopy = logSegments.toBuffer
// For the earliest and latest, we do not need to return the timestamp.
if (targetTimestamp == ListOffsetRequest.EARLIEST_TIMESTAMP)
return Some(TimestampOffset(RecordBatch.NO_TIMESTAMP, logStartOffset))
return Some(new TimestampAndOffset(RecordBatch.NO_TIMESTAMP, logStartOffset,
maybeLeaderEpoch(leaderEpochCache.earliestEpoch)))
Copy link
Contributor

Choose a reason for hiding this comment

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

The message format may have been changed after logStartOffset. In that case, leaderEpochCache.earliestEpoch may not match logStartOffset. So, we probably want to also make sure the offset corresponding to leaderEpochCache.earliestEpoch matches logStartOffset.

@hachikuji hachikuji force-pushed the KAFKA-7568 branch 2 times, most recently from 67a68c5 to ab5552e Compare November 1, 2018 17:46
@hachikuji
Copy link
Author

Thanks @junrao. Pushed an update to address your comment.

Copy link
Contributor

@junrao junrao left a comment

Choose a reason for hiding this comment

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

@hachikuji : Thanks for the updated patch. LGTM.

@hachikuji
Copy link
Author

Thanks @junrao. I will go ahead and merge. Note that we are investigating the jdk11 failures in https://issues.apache.org/jira/browse/KAFKA-7553 and elsewhere.

@hachikuji hachikuji merged commit fc1dc35 into apache:trunk Nov 1, 2018
pengxiaolong pushed a commit to pengxiaolong/kafka that referenced this pull request Jun 14, 2019
As part of KIP-320, the ListOffsets API should return the leader epoch of any fetched offset. We either get this epoch from the log itself for a timestamp query or from the epoch cache if we are searching the earliest or latest offset in the log. When handling queries for the latest offset, we have elected to choose the current leader epoch, which is consistent with other handling (e.g. OffsetsForTimes).

Reviewers: Jun Rao <junrao@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants