-
Notifications
You must be signed in to change notification settings - Fork 14.9k
KAFKA-12154: Snapshot Loading API #10085
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-12154: Snapshot Loading API #10085
Conversation
raft/src/main/java/org/apache/kafka/raft/ReplicatedCounter.java
Outdated
Show resolved
Hide resolved
raft/src/main/java/org/apache/kafka/raft/ReplicatedCounter.java
Outdated
Show resolved
Hide resolved
raft/src/main/java/org/apache/kafka/raft/metadata/MetaLogRaftShim.java
Outdated
Show resolved
Hide resolved
raft/src/main/java/org/apache/kafka/snapshot/SnapshotReader.java
Outdated
Show resolved
Hide resolved
raft/src/main/java/org/apache/kafka/raft/ReplicatedCounter.java
Outdated
Show resolved
Hide resolved
raft/src/main/java/org/apache/kafka/raft/metadata/MetaLogRaftShim.java
Outdated
Show resolved
Hide resolved
shell/src/main/java/org/apache/kafka/shell/MetadataNodeManager.java
Outdated
Show resolved
Hide resolved
|
Update the PR's description to match the latest changes included here. |
mumrah
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the patch! First pass of questions/comments inline
raft/src/main/java/org/apache/kafka/raft/ReplicatedCounter.java
Outdated
Show resolved
Hide resolved
raft/src/main/java/org/apache/kafka/raft/ReplicatedCounter.java
Outdated
Show resolved
Hide resolved
raft/src/main/java/org/apache/kafka/raft/internals/SerdeRecordsIterator.java
Outdated
Show resolved
Hide resolved
raft/src/main/java/org/apache/kafka/raft/internals/SerdeRecordsIterator.java
Show resolved
Hide resolved
raft/src/main/java/org/apache/kafka/raft/internals/SerdeRecordsIterator.java
Show resolved
Hide resolved
raft/src/main/java/org/apache/kafka/snapshot/RawSnapshotReader.java
Outdated
Show resolved
Hide resolved
raft/src/main/java/org/apache/kafka/raft/ReplicatedCounter.java
Outdated
Show resolved
Hide resolved
raft/src/main/java/org/apache/kafka/raft/internals/RecordsBatchReader.java
Outdated
Show resolved
Hide resolved
raft/src/main/java/org/apache/kafka/raft/internals/SerdeRecordsIterator.java
Outdated
Show resolved
Hide resolved
raft/src/main/java/org/apache/kafka/raft/internals/SerdeRecordsIterator.java
Outdated
Show resolved
Hide resolved
raft/src/test/java/org/apache/kafka/raft/RaftEventSimulationTest.java
Outdated
Show resolved
Hide resolved
raft/src/main/java/org/apache/kafka/raft/internals/SerdeRecordsIterator.java
Outdated
Show resolved
Hide resolved
10cd065 to
029fea5
Compare
|
@mumrah @hachikuji @dengziming thanks for the feedback. I should have addressed all of your suggestions. Let me know if I missed anything. |
raft/src/main/java/org/apache/kafka/raft/internals/RecordsIterator.java
Outdated
Show resolved
Hide resolved
raft/src/test/java/org/apache/kafka/raft/RaftClientTestContext.java
Outdated
Show resolved
Hide resolved
raft/src/test/java/org/apache/kafka/raft/internals/RecordsIteratorTest.java
Outdated
Show resolved
Hide resolved
hachikuji
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks for the patch!
Implement Raft Snapshot loading API.
Adds a new method
handleSnapshottoraft.Listenerwhich is called whenever theRaftClientdetermines that theListenerneeds to load a new snapshot before reading the log. This happens when theListener's next offset is less than the log start offset also known as the earliest snapshot.Adds a new type
SnapshotReader<T>which provides aIterator<Batch<T>>interface and de-serializes records in theRawSnapshotReaderintoTsAdds a new type
RecordsIterator<T>that implements anIterator<Batch<T>>by scanning aRecordsobject and deserializes the batches and records intoBatch<T>. This type is used by bothSnapshotReader<T>andRecordsBatchReader<T>internally to implement theIteratorinterface that they expose.Changes the
MockLogimplementation to read one batch at a time. The previous implementation always read from the given offset to the high-watermark. This made it impossible to test interesting snapshot loading scenarios.Removed
throws IOExceptionfrom some methods. Some of types were inconsistently throwingIOExceptionin some cases and throwingRuntimeException(..., new IOException(...))in others. This PR improves the consistent by wrappingIOExceptioninRuntimeExceptionin a few more places and replacingCloseablewithAutoCloseable.Updated the Kafka Raft simulation test to take into account snapshot.
ReplicatedCounterwas updated to generate snapshot after 10 records get committed. This means that theConsistentCommittedDatavalidation was extended to take snapshots into account.More detailed description of your change,
if necessary. The PR title and PR message become
the squashed commit message, so use a separate
comment to ping reviewers.
Summary of testing strategy (including rationale)
for the feature or bug fix. Unit and/or integration
tests are expected for any behaviour change and
system tests should be considered for larger changes.
Committer Checklist (excluded from commit message)