[FLINK-9799] Generalize and unify state meta info snapshot #6308
Conversation
|
CC @azagrebin |
sihuazhou
left a comment
sihuazhou
left a comment
There was a problem hiding this comment.
Looks good from my side.
| public int[] getCompatibleVersions() { | ||
| // we are compatible with version 3 (Flink 1.3.x) and version 1 & 2 (Flink 1.2.x) | ||
| return new int[] {VERSION, 3, 2, 1}; | ||
| return new int[]{VERSION, 4, 3, 2, 1}; |
There was a problem hiding this comment.
nit: miss a ' ' between '[]' and '{'.
There was a problem hiding this comment.
Both styles are ok and used in Flink, so I will stick to this.
StefanRRichter
force-pushed
the
FLINK-9799-generalize-state-meta-pr
branch
from
5e44f75 to
dcf7756
Compare
azagrebin
left a comment
azagrebin
left a comment
There was a problem hiding this comment.
Nice simplification, LGTM 👍
I left couple of suggestions to consider before merge.
|
|
||
| @Override | ||
| public int[] getCompatibleVersions() { | ||
| // we are compatible with version 3 (Flink 1.5.x), 2 (Flink 1.4.x, Flink 1.3.x) and version 1 (Flink 1.2.x) |
There was a problem hiding this comment.
was this comment redundant?
| throw new IllegalArgumentException("Unsupported read version for state meta info: " + readVersion); | ||
| } | ||
|
|
||
| switch (stateTypeHint) { |
There was a problem hiding this comment.
One small suggestion here is to move all the legacy stuff into separate package/class and leave here only call to legacy reader factory. That would unload this class a bit and simplify cleaning of legacy stuff.
| Assert.assertTrue(meta.getStateSerializer() instanceof UnloadableDummyTypeSerializer); | ||
| Assert.assertEquals(namespaceSerializer.snapshotConfiguration(), meta.getNamespaceSerializerConfigSnapshot()); | ||
| Assert.assertEquals(stateSerializer.snapshotConfiguration(), meta.getStateSerializerConfigSnapshot()); | ||
| for (StateMetaInfoSnapshot meta : serializationProxy.getStateMetaInfoSnapshots()) { |
There was a problem hiding this comment.
Change like
RegisteredKeyedBackendStateMetaInfo.Snapshot<?, ?> meta -> RegisteredKeyedBackendStateMetaInfo(StateMetaInfoSnapshot)
and then using original assertions might be more complete version of test.
Also for other meta wrappers in further tests
There was a problem hiding this comment.
👍 Good point
|
@sihuazhou @azagrebin thanks guys for the fast reviews! Will address the comments and merge. |
4 participants
What is the purpose of the change
This PR generalizes and unifies the de/serialization of state meta information in backends. We replace the snapshots and reader/writers of the individual state types with a general
StateMetaInfoSnapshotand the correspondingStateMetaInfoSnapshotReadersWriters. Backwards compatibility is maintained.Verifying this change
This change is already covered by existing tests.
Does this pull request potentially affect one of the following parts:
@Public(Evolving): (no)Documentation