Skip to content

[FLINK-37553] Make ForStKeyedStateBackend update NamespaceSerializer during restoring state#26416

Merged
Zakelly merged 2 commits into
apache:masterfrom
mayuehappy:FLINK-37553
Apr 9, 2025
Merged

[FLINK-37553] Make ForStKeyedStateBackend update NamespaceSerializer during restoring state#26416
Zakelly merged 2 commits into
apache:masterfrom
mayuehappy:FLINK-37553

Conversation

@mayuehappy
Copy link
Copy Markdown
Contributor

…during restoring state

What is the purpose of the change

Make ForStKeyedStateBackend update NamespaceSerializer during restoring state

Brief change log

  • add updateNamespaceSerializer in RegisteredKeyValueStateBackendMetaInfo
  • invoke RegisteredKeyValueStateBackendMetaInfo#updateNamespaceSerializer when creating State

Verifying this change

(Please pick either of the following options)

This change is a trivial rework

This change added tests and can be verified as follows:

add unit test ForStStateMigrationTest#testStateNamespaceSerializerChanged

Does this pull request potentially affect one of the following parts:

  • Dependencies (does it add or upgrade a dependency): (no)
  • The public API, i.e., is any changed class annotated with @Public(Evolving): (no)
  • The serializers: (no)
  • The runtime per-record code paths (performance sensitive): (no)
  • Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: (checkpointing)
  • The S3 file system connector: (no)

Documentation

  • Does this pull request introduce a new feature? (no)
  • If yes, how is the feature documented? (not documented)

@flinkbot
Copy link
Copy Markdown
Collaborator

flinkbot commented Apr 8, 2025

CI report:

Bot commands The @flinkbot bot supports the following commands:
  • @flinkbot run azure re-run the last Azure build

@Nonnull
public TypeSerializerSchemaCompatibility<N> updateNamespaceSerializer(
TypeSerializer<N> newNamespaceSerializer) {
return namespaceSerializerProvider.registerNewSerializerForRestoredState(
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.

nit: is the New in the method name required?
registerNewSerializerForRestoredState -> registerSerializerForRestoredState

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.

Well this is the name of existing interface, we'd better keep this in current PR.


TypeSerializerSchemaCompatibility<N> s =
restoredKvStateMetaInfo.updateNamespaceSerializer(namespaceSerializer);
if (s.isCompatibleAfterMigration() || s.isIncompatible()) {
Copy link
Copy Markdown
Contributor

@davidradl davidradl Apr 8, 2025

Choose a reason for hiding this comment

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

I am curious about the difference between isCompatibleAfterMigration() and isIncompatible(). If the difference is meaningful, should we include these different incompatibilities in the error message to help make the message more useful?

It would even better if the compatibility checks returned the reason for the incompatibility. Maybe throwing an exception in a checkIncompatibility method and construct different messages in the Exception might be better as we then log out the root cause of the incompatibility. WDYT?

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.

@mayuehappy We could distinguish the messages for two cases. For the isCompatibleAfterMigration, we may say 'The new namespace serializer () can be compatible with the old one () after migration. But ForSt does not support state migration yet.'

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.

@Zakelly Thanks for the reply. As I remember, NamespaceSerializer should not support migration, even RocksdbStateBackend does. So do we still need to distinguish here?

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.

@mayuehappy we can keep it if so.

Copy link
Copy Markdown
Contributor

@fredia fredia left a comment

Choose a reason for hiding this comment

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

@mayuehappy Thanks for the PR, LGTM

…g/apache/flink/state/forst/ForStStateMigrationTest.java

Co-authored-by: Yanfei Lei <fredialei@gmail.com>
@Zakelly Zakelly merged commit 40ffc15 into apache:master Apr 9, 2025
yanand0909 pushed a commit to yanand0909/flink that referenced this pull request Jun 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants