-
Notifications
You must be signed in to change notification settings - Fork 13.9k
[FLINK-34120][core] Introduce unified serialization config option for all Kryo, POJO and customized serializers #24182
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
…ryo, POJO and customized serializers
flink-core/src/main/java/org/apache/flink/api/common/serialization/SerializerConfig.java
Show resolved
Hide resolved
flink-core/src/main/java/org/apache/flink/api/common/serialization/SerializerConfig.java
Outdated
Show resolved
Hide resolved
|
@reswqa Could you help review it? Thx~ |
JunRuiLee
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 @X-czh for contribution, I've left three minor comment. PTAL.
flink-core/src/main/java/org/apache/flink/api/common/serialization/SerializerConfig.java
Outdated
Show resolved
Hide resolved
flink-core/src/main/java/org/apache/flink/api/common/serialization/SerializerConfig.java
Outdated
Show resolved
Hide resolved
flink-core/src/main/java/org/apache/flink/api/common/serialization/SerializerConfig.java
Show resolved
Hide resolved
reswqa
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 @X-czh, overall looks good to me. I only left a few comments. We need to manually verify that the previous and current way both work.
flink-core/src/main/java/org/apache/flink/api/common/serialization/SerializerConfig.java
Outdated
Show resolved
Hide resolved
flink-core/src/main/java/org/apache/flink/api/common/serialization/SerializerConfig.java
Show resolved
Hide resolved
| + " sure that only tags are written.") | ||
| .build()); | ||
|
|
||
| public static final ConfigOption<Map<String, String>> SERIALIZATION_CONFIG = |
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.
Could you please post a screenshot corresponding to the rendered config option.
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.
reswqa
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 update. +1 for merging.
JunRuiLee
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
From the unstable UT, I just realized that the config option has to be a list of pairs instead of a map of pairs since the registration order is important for POJO and Kryo serializers. I'll append a fix commit |


What is the purpose of the change
Introduce unified serialization config option for all Kryo, POJO and customized serializers for parameterized serialization config based on pure config.
Brief change log
pipeline.serialization-config, a unified serialization config option for all Kryo, POJO and customized serializers.SerializerConfig#registerPojoType,SerializerConfig#registerKryoType,SerializerConfig#addDefaultKryoSerializer, andSerializerConfig#registerTypeWithKryoSerializermethods.TypeExtractorfor now so that it can be accessed from the static methods ofTypeExtractorwhereSerializerConfigis currently not accessible. Plan to migrate the static methods to takeSerializerConfigas one of the arguments in v1.20.Verifying this change
Added tests in
SerializerConfigTeston parsing different types of serializer config, both legal and illegal.Does this pull request potentially affect one of the following parts:
@Public(Evolving): (yes / no) yesDocumentation