azagrebin commented on issue #8416: [FLINK-12331] Introduce partition/gate setup to decouple task registration with NetworkEnvironment URL: https://github.com/apache/flink/pull/8416#issuecomment-491830843 Thanks for the review @zhijiangW ! I addressed the smaller comments. The order of commits seems to have improved now :) - As for `PartitionBufferPoolFactory`, it is specific for partitions and `BufferPoolFactory` is a general one. `PartitionBufferPoolFactory` can be also seen as wrapping `BufferPoolFactory` with the partition concern. I do not think we should include partition type or something to the `BufferPoolFactory`. The `PartitionBufferPoolFactory.create` basically transforms partition type and network configuration into general parameters of `BufferPoolFactory` so I think they belong to different level of abstractions. Alternatively, we could just add more config parameters to partition/gate constructors (config and `BufferPoolFactory`) and keep `PartitionBufferPoolFactory` logic inside but this would explode the constructors and partition/gate classes even more. It should not hurt to have each class to address as less concerns as possible. - Commits for `PartitionBufferPoolFactory` factories address a certain concern, independent from setup commits. They can be compiled and tested independently as well. Not sure how temporary they are. At least, it should simplify review. We can of course squash them before merge. - `ResultPartitionFactory` encapsulates all components which are needed to create any partition. `ResultPartitionFactory.create` accepts only partition specific parameters. Overall I think it should reduce creation of partition in place and should not hurt to have this concern separately. I will also move creation of subpartitions into the factory.
---------------------------------------------------------------- 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] With regards, Apache Git Services
