becketqin commented on pull request #12122: URL: https://github.com/apache/flink/pull/12122#issuecomment-629811821
@StephanEwen Sorry for the back and forth, after second thought, I feel it might makes sense to put the `instanceOf CoordinatedOperatorFactory` in `StreamNode` instead of in the `PhysicalTransformation`. The reason being is following: 0. `CoordinatedOperatorFactory` is an implementation detail of a `StreamOperatorFactory`, instead of a type of it. It is orthogonal to and can be implemented by any type of other `StreamOperatorFactory`. 1. Conceptually speaking, `PhysicalTransformation` only cares about the type of the operator factory, it does not care about the implementation, i.e. whether the operator has a coordinator. Therefore the only thing `PhysicalTransformation` can do is to extract the coordinator provider and pass it to the `StreamGraphGenerator`. It seems much simpler and more intuitive to just let `StreamNode` to do the `instanceOf` and extract the coordinator provider factory in a method like ``` Optional<OperatorCoordinator.Provider> getOperatorCoordinatorProvider(String operatorName, OperatorID operatorID) ``` 2. Another important benefit is that this makes sure we only have one single place that needs to care about `CoordinatedProviderFactory`, which is much less error prone. The last commit updated the patch with this change. What do you think? If you have concern for it, we can merge the PR without the last commit and make the change later if we would like to. ---------------------------------------------------------------- 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]
