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

MINOR: Introduce OffsetAndEpoch in LeaderEndpoint interface return values #13268

Merged
merged 5 commits into from
Feb 24, 2023

Conversation

kowshik
Copy link
Contributor

@kowshik kowshik commented Feb 17, 2023

Some of the LeaderEndpoint interface methods used a tuple of (Int, Long) as return value to represent epoch & offset. In this PR, I have replaced those occcurences with the existing OffsetAndEpoch type. This should hopefully improve code readability, and eliminate the chances of developer confusing offset with epoch (or vice versa) at the call site or return site.

Tests:
Rely on existing tests

@kowshik
Copy link
Contributor Author

kowshik commented Feb 17, 2023

Hi @junrao and @satishd -- Please could you help review this PR?

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.

@kowshik : Thanks for the PR. Left a comment. Also, the built failed with unused import.


package kafka.server

case class OffsetAndEpoch(offset: Long, leaderEpoch: Int) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we add this in the server-common module in java?

@kowshik kowshik force-pushed the cleanup_leaderendpoint_OffsetAndEpoch branch 2 times, most recently from 02b0518 to 99cafb8 Compare February 20, 2023 23:01
@kowshik
Copy link
Contributor Author

kowshik commented Feb 20, 2023

@junrao Thanks for the review! I've addressed the comments, the PR is ready for another pass.

Copy link
Member

@satishd satishd left a comment

Choose a reason for hiding this comment

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

Thanks @kowshik for the updated PR, LGTM.

warn(s"Reset fetch offset for partition $topicPartition from $replicaEndOffset to current " +
s"leader's start offset $leaderStartOffset")
val offsetToFetch =
if (leaderStartOffset > replicaEndOffset) {
// Only truncate log when current leader's log start offset (local log start offset if >= 3.4 version incaseof
// OffsetMovedToTieredStorage error) is greater than follower's log end offset.
// truncateAndBuild returns offset value from which it needs to start fetching.
truncateAndBuild(epoch, leaderStartOffset)
truncateAndBuild(offsetAndEpoch.leaderEpoch, leaderStartOffset)
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor - maybe the argument of truncateAndBuild could be the offsetAndEpoch itself?

Copy link
Contributor Author

@kowshik kowshik Feb 21, 2023

Choose a reason for hiding this comment

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

Done in 3da1e63. Good point!

@kowshik
Copy link
Contributor Author

kowshik commented Feb 21, 2023

@Hangleton Thanks for the review. I have addressed the comment, the PR is ready for another pass.

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.

@kowshik : Thanks for the updated PR. Could you rebase?

@kowshik kowshik force-pushed the cleanup_leaderendpoint_OffsetAndEpoch branch from 3da1e63 to 484eef1 Compare February 22, 2023 23:04
@kowshik
Copy link
Contributor Author

kowshik commented Feb 22, 2023

@junrao Thanks for the review. I have rebased the PR and brought in the latest commits from AK trunk. PR is ready for a pass.

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.

@kowshik : Thanks for rebasing the PR. LGTM

@junrao junrao merged commit 9f55945 into apache:trunk Feb 24, 2023
@Hangleton
Copy link
Contributor

Thanks!

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.

4 participants