guoweiM edited a comment on pull request #12132: URL: https://github.com/apache/flink/pull/12132#issuecomment-629923445
HI @aljoscha , 1. First of all, I think you are right we should merge to master after all the participants agree with the pr. 2. I could split the commit to two commits: Update BucketStateSerierlizerTest and Rework. I think it is a good practice. Before do it I want to agree on how to change it. Using the `Bucket` not `InProgressFileWriter` or `RecoverableFsDataOutputStream` to produce pending-file/in-progress-file. The other two main concerns are the following. 3. I would want to share some thoughts about using a special `RcoverableWriter` tests the buck state migration. Currently, `Bucket`/`BucketState`/`BucketStateSerializer` depends on interface, not some special implementation. IMO any implementation of `RecoverableWriter` is a ‘special’ one. The code path we want to test is bucket state(pendingFile&inProgressWriter) serializer(snapshot) and deserializer(restore). Using which implementation depends on convenience. For example, we always use the LocalFileSystem does not use the S3 or HDFS. So I think it is ok if the implementation could test the code path. 4. For custom file copying/cleanup things. I think it is important that these PendingFile/InProgressFileWriter that are restored from v1 or v2 bytes should be work really. That the old version of `BucketStateSerierlizerTest` also verifies this. It’s why we do that. Of course, this is all my personal thoughts. Correct if I missing something. ---------------------------------------------------------------- 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
