pnowojski commented on pull request #13109: URL: https://github.com/apache/flink/pull/13109#issuecomment-684919457
Thanks @liming30 for the update. It's a bit difficult for me to review this now, because of the commits structure. I can not review the first commit, as it is being fixed by the 3rd and 4th commit, while I can not review all commits together, because 2nd commit and 3rd commits contain non functional changes (refactoring). The 3rd commit is both fixing the first commit and it also contains a clean up of removing inefficient `Set<...> selectOutputs()` method. Could you restructure first three commits in the following way ([following our coding style](https://flink.apache.org/contributing/code-style-and-quality-pull-requests.html#separate-refactoring-cleanup-and-independent-changes)): 1. my `[hotfix][task] Move output and collector helper classes out of OperatorChain` commit 2. your introduction of `SelectedOutputsCollector` - a hotfix/optimisation of the pre-existing code 3. your functional changes to the metrics counting (current 1st and part of the current 3rd commit) 4. your `[hotfix] wrapping single RecordWriterOutput with RecordWriterCountingOutput` I'm not entirely sure if the 4th commit should be squashed with the 3rd or not, but that can be done easily afterwards if needed. What's currently causing me most problems while trying to review are the first three commits. ---------------------------------------------------------------- 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]
