Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/6235
Took a look at this WIP and I think it goes into a good direction.
My most important comment is that I think it would help to move the
"ensureCompatibility" into the config snapshot, for the following reasons:
- Clearer separation of concerns, the serializer has only the
serialization logic, and creating the snapshot. Compatibility is not the
serializers immediate concern.
- The current design means that the serializer mutates internal fields on
reconfiguration. That is error prone. Consider a serializer like the
KryoSerializer, where the configuration is not fully deep copied on duplication
(makes sense, it is read only during serialization). Mutating that
configuration would change the behavior of other previously duplicated
serializers as well, which is unexpected.
Thoughts for improvements with lower priority:
- Can we avoid setting the ClassLoader into a field in the config
snapshot, and then deserializing? I think such solutions are fragile and should
be avoided if possible. The ClassLoader is not really part of the snapshots
state, it is an auxiliary to the deserialization and should, as such, be passed
as an argument to the read method: read(in, classloader). This means that the
TypeSerializerConfigSnapshot would not implement `IOReadableWritable`, but that
might be not a problem.
- Is the TypeSerializerConfigSnapshotSerializationProxy needed? It seems
like an unnecessary indirection given that it is used exclusively in the
TypeSerializerSerializationUtil and could be a static util method instead.
---
