zhijiangW 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-492070855 @azagrebin thanks for the confirmation. As for `PartitionBufferPoolFactory`: - The current `BufferPoolFactory` exists only for partition/gate, and no other parts would need `BufferPool` in flink stack atm. The new introduced `PartitionBufferPoolFactory` is also for partition/gate, but has no relationship with `BufferPoolFactory` in the architecture form. So I think it is not very specific for existing two parallel interfaces related with `BufferPool`. - We could also pass the `ResultPartitionType` in constructor of `AbstractPartitionBufferPoolFactory`, then the new proposed interface method is shown as `create(int size, BufferPoolOwner owner)`, so the only difference with previous methods in `BufferPoolFactory` is we do not pass the `maxUsedBuffers` which is calculated based on `ResultPartitionType`. - The key motivation of proposing new interface is we could only pass one parameter factory in constructor of `ResultPartition`, otherwise we need two parameters of `NetworkEnvironmentConfiguration` and `BufferPoolFactory`. But for the constructor of `SingleInputGate`, it seems the same because we could use `NetworkEnvironmentConfiguration` to replace the current `isCreditBased` parameter. - Another possible advantage of new interface is we extract the logic of creating `BufferPool` to another place, then it makes simple for `SingleInputGate`,`ResultPartition` and `NetworkEnvironment`. - From the aspect of extended architecture, it seems no problem if we did nothing. Because different `ShuffleService` implementations might need different `InputGate` and `ResultPartitionWriter` implementations. Even we might extract the current `BlockingSubpartition` as a separate implementation of `ResultPartitionWriter` in future, and it does not need `BufferPoolFactory` any more. In summary I do not think it is very necessary to introduce new interface here, or not very worthing. If we want to make constructor of partition/gate simple, we could only pass `Supplier<BufferPool>` as you suggested before, and it could be explained by `NetworkEnvironment` itself. If the way of `Supplier` seems not good, I have another option: - Add new method `createBufferPool(int numRequiredBuffers, BufferPoolOwner owner)` in existing `BufferPoolFactory`. - Make `AbstractPartitionBufferPoolFactory` implements `BufferPoolFactory` and we could also pass `ResultPartitionType` into constructor of `AbstractPartitionBufferPoolFactory`. - `ResultPartitionBufferPoolFactory/InputGateBufferPoolFactory` extends `AbstractPartitionBufferPoolFactory` and they only need to implement the new method `createBufferPool(int numRequiredBuffers, BufferPoolOwner owner)`. To do so we extend and reuse the general `BufferPoolFactory` and only abstract/distinguish the implementation of calculating `maxPoolSize` in partition/gate. What do you think?
---------------------------------------------------------------- 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 With regards, Apache Git Services
