StephanEwen commented on pull request #11722: URL: https://github.com/apache/flink/pull/11722#issuecomment-629557807
@klion26 I did a pretty thorough review. Overall, the approach is seems good. I have a set of suggestions for follow-ups, which I put in a PR for your convenience: https://github.com/apache/flink/pull/12187 Most changes are small and should be straightforward. Onyl one change is bigger, the reworking of the `MetadataV2V3SerializerBase` (together with the `MetadataV3Serializer`). That change has two main goals: 1. Change the pointer / path resolution caching from a static variable to a parameter that is passed. The serializers are singleton instances and currently assume to be usable in a multi-threaded manner. The static variables prevent this, usng the context object parameter restores this behavior. 2. (this was a pre-existing issue) Lower level serialization methods should not expose themselves directly to the tests, because that bypasses the versioning scheme. That leads to tests that check implementation instead of behavior. The change here makes (almost) all lower level serialization methods instance methods and package-private. Static access methods are gathered in one place, as a workaround for tests that require access to those lower level methods (even through they should not). With more unified access to the methods from tests, we can now make prevent that tests need to be aware of the the context object or external pointer parameter. Event though this was a pre-existing problem, I added the fix here, because your PR needed to adjust all the method calls in tests, and this change introduced a way to shield the tests from further changes. ---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: [email protected]
