[KAFKA-13369] Follower fetch protocol changes for tiered storage. by satishd · Pull Request #11390 · apache/kafka

satishd · 2021-10-12T10:19:44Z

This PR implements the follower fetch protocol as mentioned in KIP-405.

Added a new version for ListOffsets protocol to receive local log start offset on the leader replica. This is used by follower replicas to find the local log star offset on the leader.

Added a new version for FetchRequest protocol to receive OffsetMovedToTieredStorageException error. This is part of the enhanced fetch protocol as described in KIP-405.

We introduced a new field locaLogStartOffset to maintain the log start offset in the local logs. Existing logStartOffset will continue to be the log start offset of the effective log that includes the segments in remote storage.

When a follower receives OffsetMovedToTieredStorage, then it tries to build the required state from the leader and remote storage so that it can be ready to move to fetch state.

Introduced RemoteLogManager which is responsible for

initializing RemoteStorageManager and RemoteLogMetadataManager instances.
receives any leader and follower replica events and partition stop events and act on them
also provides APIs to fetch indexes, metadata about remote log segments.

Followup PRs will add more functionality like copying segments to tiered storage, retention checks to clean local and remote log segments. This will change the local log start offset and make sure the follower fetch protocol works fine for several cases.

You can look at the detailed protocol changes in KIP: https://cwiki.apache.org/confluence/display/KAFKA/KIP-405%3A+Kafka+Tiered+Storage#KIP405:KafkaTieredStorage-FollowerReplication

Authors: satishd@apache.org, kamal.chandraprakash@gmail.com, yingz@uber.com

Committer Checklist (excluded from commit message)

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

kowshik · 2021-11-01T18:01:21Z

@satishd It'd be helpful if you could please update the PR description explaining the scope of the draft PR (in its current form) and what's remaining to be done.

satishd · 2021-11-17T16:18:40Z

@junrao @kowshik This PR is ready for review. Please take a look.

junrao

@satishd : Thanks for the PR. Made a pass of all non-testing files. A few comments below.

satishd · 2021-12-13T15:55:04Z

Thanks @junrao for the review. Please find inline replies, addressed most of them with latest commits.

junrao

@satishd : Thanks for the updated PR. A few more comments below.

junrao · 2021-12-20T06:38:21Z

-
+                case Errors.OFFSET_MOVED_TO_TIERED_STORAGE =>
+                  // No need to retry this as it indicates that the requested offset is moved to tiered storage.
+                  // Check whether topicId is available here.


Was this comment addressed?

junrao · 2021-12-20T06:38:43Z

The return value is not very intuitive. I'd expect a true return value to indicate that the request is handled successfully.

This is inline with handleOutOfRangeError contract. I am fine with the suggested change but it is good to have similar semantics to handleOutOfRangeError method too for uniformity.

satishd · 2021-12-21T14:20:51Z

@junrao : Thanks for the review. Please find inline replies, updated the PR addressing them with the latest commit.

Summary: Added more UTs for RemoteLogManager Test Plan: UT Reviewers: abhijeek, satishd Reviewed By: satishd Revert Plan: git revert API Changes: NA Monitoring and Alerts: NA

Summary: - The latest metadata version IBP_3_3_IV4 was not referred on the existing tests which lead to the failures. - After the local and remote leader endpoint changes, the fetcher thread tests related to remote storage become stale, fixed them. - Fixed the issue while renaming the parent directory of the index files. Test Plan: ./gradlew :core:test ./gradlew :metadata:test Reviewers: abhijeek, satishd Revert Plan: git revert API Changes: NA Monitoring and Alerts: NA

- Added loading of the existing indexes in the disk and added respective test. - Addressed other minor comments.

…h setting the right metadata max version

…r updates

Resolved a few test cases related to new versions.

…amp as the first message timestamp

…or getIndex if it is already closed. Addressed review comments

Updated highwatermark when producerstate is rebuilt in fetcher. Throwing RemoteStorageException from follwer replica when remtoe stoage is not yet enabled.

…emoteLogManagerTest

satishd · 2022-12-16T10:45:06Z

Thanks @junrao for your updated review. Addressed them with inline comments and updated with the latest commits.

junrao

@satishd : Thanks for the updated PR. Just one more comment. Also, there are quite a few test failures. Are they related to this PR?

junrao · 2022-12-16T20:06:38Z

+  def loadProducerState(lastOffset: Long): Unit = lock synchronized {
+    rebuildProducerState(lastOffset, producerStateManager)
+    maybeIncrementFirstUnstableOffset()
+    updateHighWatermark(localLog.logEndOffset)


logEndOffset still uses offset. We want to use logEndOffsetMetadata.

…n UnifiedLog#loadProducerState

satishd · 2022-12-17T13:12:53Z

Thanks @junrao for the review, addressed it with the latest commit.

There are a few tests that are failed but they do not seem to be related to this PR.

junrao

@satishd : Thanks for the latest PR. LGTM

ijuma · 2022-12-17T19:00:58Z

Hey @satishd, I wanted to let you know about KAFKA-14470 as I think it affects some of the future KIP-405 PRs. Can we align these efforts so that we can get to the desired end state faster? For example, once the PRs that have been submitted are merged, we can move RemoteIndexCache to the storage module too.

satishd force-pushed the tier-follower-fetch branch 4 times, most recently from bc2983c to 270988f Compare October 14, 2021 08:41

satishd force-pushed the tier-follower-fetch branch from 270988f to de8ef09 Compare October 19, 2021 11:58

satishd force-pushed the tier-follower-fetch branch 5 times, most recently from f0e0b61 to 9d6e2a9 Compare November 17, 2021 09:53

satishd marked this pull request as ready for review November 17, 2021 10:33

ccding reviewed Nov 17, 2021

View reviewed changes

Comment thread core/src/main/scala/kafka/log/CleanableIndex.scala Outdated

Comment thread core/src/main/scala/kafka/server/BrokerServer.scala Outdated

Comment thread core/src/main/scala/kafka/server/ReplicaFetcherThread.scala Outdated

satishd force-pushed the tier-follower-fetch branch from 9d6e2a9 to 1436fb3 Compare November 18, 2021 04:14

junrao reviewed Dec 4, 2021

View reviewed changes

satishd force-pushed the tier-follower-fetch branch 3 times, most recently from 611244c to eabd343 Compare December 13, 2021 12:23

junrao reviewed Dec 20, 2021

View reviewed changes

satishd force-pushed the tier-follower-fetch branch 3 times, most recently from baa5b72 to 0c0b477 Compare December 21, 2021 13:42

wyuka reviewed Dec 21, 2021

View reviewed changes

Comment thread core/src/main/java/kafka/server/builders/ReplicaManagerBuilder.java Outdated

satishd force-pushed the tier-follower-fetch branch 3 times, most recently from c5830c2 to 2a69bea Compare December 22, 2021 08:25

satishd mentioned this pull request Dec 22, 2021

Added checkpoint truncation utility with inmemory writers instead of temporary files in RemoteLogManager. #11623

Closed

3 tasks

kamalcph and others added 21 commits December 16, 2022 15:27

Added more UTs for RemoteLogManager

b8700e3

Summary: Added more UTs for RemoteLogManager Test Plan: UT Reviewers: abhijeek, satishd Reviewed By: satishd Revert Plan: git revert API Changes: NA Monitoring and Alerts: NA

Fixed QuorumControllerTest with the right updated MetadataVersion

7bcf4df

Fixed UnifiedLogTest.testNoOpWhenRemoteLogStorageIsDisabled

c322ca9

Addressed review comments

d78c62b

- Added loading of the existing indexes in the disk and added respective test. - Addressed other minor comments.

Refactored existing index classes by removing BaseIndex class

9a49da3

Fixed QuorumControllerTest#testSnapshotAfterConfiguredMaxInterval wit…

848eac7

…h setting the right metadata max version

Extracted add(Seq[EpochEntry]) to LeaderEpochFileCache and othe rmino…

4afd6b0

…r updates

Fixed indentations and minor typos

8e19c34

Minor refactoring and added a few more docs.

a15ecf8

Resolved conflicts with the latest trunk updates.

d8cc402

Resolved a few test cases related to new versions.

Updated the java doc to make it clear as the returned offset's timest…

219399b

…amp as the first message timestamp

Cleanedup RemoteIndexCache with javadoc and added a defensive check f…

33104d0

…or getIndex if it is already closed. Addressed review comments

Removed unused import

7236246

Addressed review comments

0da2262

Addressed review comments

2412703

Addressed review comments

59ca3d7

Updated highwatermark when producerstate is rebuilt in fetcher. Throwing RemoteStorageException from follwer replica when remtoe stoage is not yet enabled.

Added documentation about new tests in ProducerStateManagerTest and R…

455ff0f

…emoteLogManagerTest

Fix a minor typo in RemoteIndexCacheTest doc

eacef4d

Addressed review comments

a8e7a39

Addressed the conflicts from trunk merge

6bb70f6

junrao reviewed Dec 16, 2022

View reviewed changes

Fixed setting localLog.logEndOffsetMetadata for updateHighWatermark i…

94c127a

…n UnifiedLog#loadProducerState

junrao approved these changes Dec 17, 2022

View reviewed changes

ijuma mentioned this pull request Dec 17, 2022

MINOR: Fix compiler errors in RemoteIndex* related to OffsetPosition #13008

Merged

3 tasks

rondagostino mentioned this pull request Feb 3, 2023

MINOR: Rename IBP_3_4_IV1 as added for KIP-405 to IBP_3_5_IV0 #13198

Merged

3 tasks

chia7712 mentioned this pull request Aug 21, 2024

KAFKA-17331: Throw unsupported version exception if the server does NOT support EarliestLocalSpec and LatestTieredSpec #16873

Merged

3 tasks

Conversation

satishd commented Oct 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Committer Checklist (excluded from commit message)

Uh oh!

kowshik commented Nov 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

satishd commented Nov 17, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

junrao left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

satishd commented Dec 13, 2021

Uh oh!

junrao left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

junrao Dec 20, 2021

Choose a reason for hiding this comment

Uh oh!

junrao Dec 20, 2021

Choose a reason for hiding this comment

Uh oh!

satishd Dec 21, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

satishd commented Dec 21, 2021

Uh oh!

Uh oh!

satishd commented Dec 16, 2022

Uh oh!

junrao left a comment

Choose a reason for hiding this comment

Uh oh!

junrao Dec 16, 2022

Choose a reason for hiding this comment

Uh oh!

satishd commented Dec 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

junrao left a comment

Choose a reason for hiding this comment

Uh oh!

ijuma commented Dec 17, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

satishd commented Oct 12, 2021 •

edited

Loading

kowshik commented Nov 1, 2021 •

edited

Loading

satishd commented Dec 17, 2022 •

edited

Loading