-
Notifications
You must be signed in to change notification settings - Fork 13.8k
[FLINK-23143][state/changelog] Support state migration for ChangelogS… #19679
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
Conversation
69b10ce to
d6adb3d
Compare
...rocksdb/src/main/java/org/apache/flink/contrib/streaming/state/RocksDBKeyedStateBackend.java
Outdated
Show resolved
Hide resolved
...changelog/src/main/java/org/apache/flink/state/changelog/restore/ChangelogRestoreTarget.java
Outdated
Show resolved
Hide resolved
rkhachatryan
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 updating the PR @masteryhx and writing the design document.
I've left some comments, PTAL (let's see if the document needs to be updated).
Please also check the cause of the build failure.
flink-runtime/src/main/java/org/apache/flink/runtime/state/heap/HeapKeyedStateBackend.java
Outdated
Show resolved
Hide resolved
...end-changelog/src/main/java/org/apache/flink/state/changelog/ChangelogKeyedStateBackend.java
Show resolved
Hide resolved
...end-changelog/src/main/java/org/apache/flink/state/changelog/ChangelogKeyedStateBackend.java
Outdated
Show resolved
Hide resolved
aa9a328 to
d9e294f
Compare
rkhachatryan
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 updating the PR @masteryhx
I've left some comments, PTAL.
flink-runtime/src/main/java/org/apache/flink/runtime/state/heap/HeapAggregatingState.java
Outdated
Show resolved
Hide resolved
flink-runtime/src/main/java/org/apache/flink/runtime/state/heap/HeapKeyedStateBackend.java
Outdated
Show resolved
Hide resolved
flink-runtime/src/main/java/org/apache/flink/runtime/state/heap/HeapKeyedStateBackend.java
Outdated
Show resolved
Hide resolved
flink-runtime/src/main/java/org/apache/flink/runtime/state/heap/HeapKeyedStateBackend.java
Outdated
Show resolved
Hide resolved
flink-runtime/src/main/java/org/apache/flink/runtime/state/heap/HeapKeyedStateBackend.java
Outdated
Show resolved
Hide resolved
.../src/main/java/org/apache/flink/state/changelog/restore/ChangelogMigrationRestoreTarget.java
Outdated
Show resolved
Hide resolved
rkhachatryan
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 updating the PR @masteryhx
I've left some comments, PTAL.
There is also a TODO associated with this ticket:
// todo: support changing ttl (FLINK-23143)
which is currently not addressed.
flink-runtime/src/main/java/org/apache/flink/runtime/state/KeyedStateFactory.java
Outdated
Show resolved
Hide resolved
...angelog/src/main/java/org/apache/flink/state/changelog/ChangelogKeyGroupedPriorityQueue.java
Outdated
Show resolved
Hide resolved
...rocksdb/src/main/java/org/apache/flink/contrib/streaming/state/RocksDBKeyedStateBackend.java
Show resolved
Hide resolved
...rocksdb/src/main/java/org/apache/flink/contrib/streaming/state/RocksDBKeyedStateBackend.java
Outdated
Show resolved
Hide resolved
...gelog/src/test/java/org/apache/flink/state/changelog/ChangelogStateBackendMigrationTest.java
Outdated
Show resolved
Hide resolved
As we discussed offline, it should be supported in the delegated state backend. |
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 updating the PR @masteryhx
Production code changes LGTM.
I have some concerns regarding the test coverage, PTAL at the comments.
Besides that, commit 86bb0c2 seems not belonging to this PR;
and commits d49670b and 05d91de should be merged together.
So in the end I think there should be just two commits:
- [refactor][state] Rename createInternalState to createOrUpdateInternalState
- [FLINK-23143][state/changelog] Support state migration for ChangelogStateBackend
WDYT?
...end-changelog/src/main/java/org/apache/flink/state/changelog/ChangelogKeyedStateBackend.java
Show resolved
Hide resolved
flink-runtime/src/main/java/org/apache/flink/runtime/state/heap/HeapKeyedStateBackend.java
Show resolved
Hide resolved
...rocksdb/src/main/java/org/apache/flink/contrib/streaming/state/RocksDBKeyedStateBackend.java
Show resolved
Hide resolved
I agree. |
rkhachatryan
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 a lot for working on this PR @masteryhx,
LGTM.
What is the purpose of the change
This pull request makes Changelog support state migration.
Brief change log
upgradeKeyedState(final TypeSerializer<N> namespaceSerializer, StateDescriptor<S, V> stateDescriptor)inAbstractKeyedStateBackendupgrade( @Nonnull String stateName, @Nonnull TypeSerializer<T> byteOrderedElementSerializer)inPriorityQueueSetFactoryVerifying this change
ChangelogStateBackendMigrationTestDoes this pull request potentially affect one of the following parts:
@Public(Evolving): noDocumentation