-
Notifications
You must be signed in to change notification settings - Fork 13.8k
[FLINK-37546] Use DefaultInstantiatorStrategy for FlinkScalaKryoInstantiator #26357
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
base: master
Are you sure you want to change the base?
Conversation
|
@mxm there are numerous table planner test failures in the CI tests. |
davidradl
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.
Please could you provide a unit test for this change.
Also the CI is failing - on the face of it due to this change.
|
Thanks for the review @davidradl! I briefly looked into adding a test but it proofed a bit difficult due to zero tests for the Scala serializer extensions. The test would have to go into the Scala module or the e2e test module. It isn't possible to test this from flink-core where KryoSerializer resides because the code is dynamically loaded from the table api scala module. I'm OoO currently but I'll take a look next week. |
|
Hi @mxm , @davidradl - is there any update on this PR? We're experiencing the same problem and it would be great to have this in flink 2.0.1. Thanks. |
|
Hey @michaelparkin! Could you share more details about your use case? When do you run into the serialization issue? |
|
Hi @mxm @davidradl , any update on the PR. We are facing similar issue with Mongodb connectors where we get null pointer since the constructor is not called by kyro. PR which has failure in build logs. apache/flink-connector-mongodb#57 Happy to help on this PR if anything is pending. |
…ntiator KryoSerializer loads FlinkScalaKryoInstantiator via Reflection which configures Kryo to use StdInstantiatorStrategy. After FLINK-3154 removed the Twitter Chill library, various types are broken. See EsotericSoftware/kryo#1173 At first, it seemed that the Kryo update caused this but I can reproduce this also with Kryo 2.24.0 with the Chill library removed. This change matches the default strategy when the Scala serializers are unavailable: https://github.com/apache/flink/blob/f7a53cede10fa25cb025d5554126a83773ba6bf9/flink-core/src/main/java/org/apache/flink/api/java/typeutils/runtime/kryo/KryoSerializer.java#L496
|
I think we need to fix some of the custom Scala serializers after this change. The current serializer configuration relies on the default strategy to be |
KryoSerializer loads FlinkScalaKryoInstantiator via Reflection which configures Kryo to use StdInstantiatorStrategy. After FLINK-3154 removed the Twitter Chill library, various types are broken. See EsotericSoftware/kryo#1173
At first, it seemed that the Kryo update caused this but I can reproduce this also with Kryo 2.24.0 with the Chill library removed.
This change matches the default strategy when the Scala serializers are unavailable: https://github.com/apache/flink/blob/f7a53cede10fa25cb025d5554126a83773ba6bf9/flink-core/src/main/java/org/apache/flink/api/java/typeutils/runtime/kryo/KryoSerializer.java#L496