[GitHub] flink issue #6235: [FLINK-9377] [core] Remove serializers from checkpointed ...

[StephanEwen <gi...@git.apache.org>]

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.



---