aljoscha commented on pull request #12132: URL: https://github.com/apache/flink/pull/12132#issuecomment-629851215
That was not very nice. I assumed it was clear that it is important that the upgrade/migration test be added separately before the changes to the serializer. As it was merged on master this is not reproducible, because there is no code for generating the v1 snapshots and both the changes to the test/snapshots/serializer itself happen in one commit. Also, the special `RecoverableWriter` in `TestUtils` meant that we use special code paths for a test, which was not needed in my version. And relying on custom file copying/cleanup in tests makes them more susceptible to subtle problems or leftover junk. Plus, there are still some issues in the code, such as raw-use of generic types. For example in most places where a `BucketStateSerializer` is created. And this is a real issue because the interfaces of what `BucketStateSerializer` expects and what `WriterProperties.getInProgressFileRecoverableSerializer()` returns do not match, even though they should. I reverted the changes on `master` and added the `BucketStateSerializer` as a commit. I also fixed the problem that made the test fail for the v2 serializer and created an updated branch that has all your changes on top of the reverts, with the code fixed to pass the test: https://github.com/aljoscha/flink/commits/pr-12132-file-sink. This could be merged, but as I said there are still some other issues in the code. I understand that we're under time pressure but rushed solo efforts are not the way to go here, especially if two reviewers still had comments/outstanding suggestions. This now caused me a lot of extra work and you have to go and look at things again and merge them. ---------------------------------------------------------------- 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: us...@infra.apache.org
