Skip to content

KAFKA-15241: Compute tiered copied offset by keeping the respective epochs in scope#14787

Merged
satishd merged 4 commits into
apache:trunkfrom
kamalcph:KAFKA-15241
Nov 27, 2023
Merged

KAFKA-15241: Compute tiered copied offset by keeping the respective epochs in scope#14787
satishd merged 4 commits into
apache:trunkfrom
kamalcph:KAFKA-15241

Conversation

@kamalcph

Copy link
Copy Markdown
Contributor

findHighestRemoteOffset does not take into account the leader-epoch end offset. This can cause log divergence between the local and remote log segments when there is unclean leader election.

To handle it correctly, the logic to find the highest remote offset can be updated to:

find-highest-remote-offset = min(end-offset-for-epoch-in-the-checkpoint, highest-remote-offset-for-epoch)

Discussion thread: #14004 (comment)

Committer Checklist (excluded from commit message)

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

@kamalcph kamalcph added the tiered-storage Related to the Tiered Storage feature label Nov 17, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

After broker restart, if there are no more segments to upload, then the copiedOffset might be stale. It's good to update the highestOffsetInRemoteStorage in the UnifiedLog once we compute it.

@clolov

clolov commented Nov 17, 2023

Copy link
Copy Markdown
Contributor

I will aim to provide a review today!

@clolov clolov left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

On a first read the change makes sense to me barring the few small comments

Comment thread core/src/test/java/kafka/log/remote/RemoteLogManagerTest.java Outdated
Comment thread core/src/test/java/kafka/log/remote/RemoteLogManagerTest.java Outdated
Comment thread storage/src/main/java/org/apache/kafka/storage/internals/log/EpochAndOffset.java Outdated
…pochs in scope.

findHighestRemoteOffset does not take into account the leader-epoch end offset. This can cause log divergence between the local and remote log segments when there is unclean leader election.

To handle it correctly, the logic to find the highest remote offset can be updated to:

find-highest-remote-offset = min(end-offset-for-epoch-in-the-checkpoint, highest-remote-offset-for-epoch)

Discussion thread: apache#14004 (comment)

@satishd satishd 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 @kamalcph for the PR, left a couple of comments.

Comment thread core/src/main/java/kafka/log/remote/RemoteLogManager.java Outdated
Comment thread core/src/main/java/kafka/log/remote/RemoteLogManager.java Outdated
@kamalcph

Copy link
Copy Markdown
Contributor Author

@satishd

Addressed your review comments. PTAL.

@satishd satishd 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 @kamalcph for addressing the review comments. LGTM.

@satishd

satishd commented Nov 27, 2023

Copy link
Copy Markdown
Member

Test failures in jenkins jobs are unrelated to this change, merging it to trunk.

@satishd
satishd merged commit a42c846 into apache:trunk Nov 27, 2023
@kamalcph
kamalcph deleted the KAFKA-15241 branch November 28, 2023 04:38
yyu1993 pushed a commit to yyu1993/kafka that referenced this pull request Feb 15, 2024
…pochs in scope (apache#14787)

"findHighestRemoteOffset" does not take into account the leader-epoch end offset. This can cause log divergence between the local and remote log segments when there is unclean leader election.

To handle it correctly, the logic to find the highest remote offset can be updated to:
find-highest-remote-offset = min(end-offset-for-epoch-in-the-checkpoint, highest-remote-offset-for-epoch)

Discussion thread: apache#14004 (comment)

Reviewers: Satish Duggana <satishd@apache.org>, Christo Lolov <lolovc@amazon.com>
AnatolyPopov pushed a commit to aiven/kafka that referenced this pull request Feb 16, 2024
…pochs in scope (apache#14787)

"findHighestRemoteOffset" does not take into account the leader-epoch end offset. This can cause log divergence between the local and remote log segments when there is unclean leader election.

To handle it correctly, the logic to find the highest remote offset can be updated to:
find-highest-remote-offset = min(end-offset-for-epoch-in-the-checkpoint, highest-remote-offset-for-epoch)

Discussion thread: apache#14004 (comment)

Reviewers: Satish Duggana <satishd@apache.org>, Christo Lolov <lolovc@amazon.com>
clolov pushed a commit to clolov/kafka that referenced this pull request Apr 5, 2024
…pochs in scope (apache#14787)

"findHighestRemoteOffset" does not take into account the leader-epoch end offset. This can cause log divergence between the local and remote log segments when there is unclean leader election.

To handle it correctly, the logic to find the highest remote offset can be updated to:
find-highest-remote-offset = min(end-offset-for-epoch-in-the-checkpoint, highest-remote-offset-for-epoch)

Discussion thread: apache#14004 (comment)

Reviewers: Satish Duggana <satishd@apache.org>, Christo Lolov <lolovc@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

tiered-storage Related to the Tiered Storage feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants