[GitHub] [flink] zhijiangW commented on issue #8310: [FLINK-12331][network] Introduce partition/gate setup to decouple task registration with NetworkEnvironment

[GitBox <gi...@apache.org>]

zhijiangW commented on issue #8310: [FLINK-12331][network] Introduce partition/gate setup to decouple task registration with NetworkEnvironment
URL: https://github.com/apache/flink/pull/8310#issuecomment-489495824
 
 
   Thanks for further reviews @azagrebin 
   
   My previous understanding of providing `Supplier` for partition/gate was not changing the existing main processes much, then the unit tests could almost work after the refactoring. 
   
   But from the aspect of circular dependency you mentioned, I guess you mean the `NetworkEnvironment` relies on `ResultPartition` and `SingleInputGate` during setup `BufferPool`, meanwhile the constructors of `ResultPartition` and `SingleInputGate` also rely on some components of `NetworkEnvironment`. 
   
   To be honest, I am not very sure what is the benefits of decoupling among them, because the `ResultPartition/SingleInputGate` are both from `NetworkEnvironment`. It seems reasonable to make them dependent with each other. 
   
   - The proposed `MemorySegmentProvider` could avoid relying on `NetworkBufferPool` in partition/gate, but partition/gate would also rely on other components `ResultPartitionManager`, `ConnectionManager`, `IOManager` etc from `NetworkEnvironment`, so it still could not solve circular dependency completely. 
   
   - We could not rely on `ResultPartition/SingleInputGate` directly in `NetworkEnvironment#setupPartition/Gate` by passing specific parameters `ResultPartitionType`, `numberOfChannels` etc. But considering not affecting tests, I have not changed this.
   
   - The proposed `MemorySegmentProvider` interface seems a bit overlap with current `BufferProvider` in semantics. Furthermore we might have only one type of implementation in `NetworkEnvironment`. So it seems not very reasonable to define a new interface to replace specific `NetworkBufferPool` here.
   
   Maybe there are any other concerns I have not caught up with? Nevertheless I update the codes based on your above suggestions of introducing `MemorySegmentProvider` and `Supplier<BufferPool>` (I have not fixed the tests), then we could further confirm whether this way is suitable. 

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services