[GitHub] [flink] zhijiangW commented on issue #7368: [FLINK-10742][network] Let Netty use Flink's buffers directly in credit-based mode

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

zhijiangW commented on issue #7368: [FLINK-10742][network] Let Netty use Flink's buffers directly in credit-based mode
URL: https://github.com/apache/flink/pull/7368#issuecomment-582745768
 
 
   I get the general idea of the implementation, but have not thought the details through yet. This improvement only validates for the body of `BufferResponse` message on receiver side, and we still have memory overheads for other messages and the frame and header parts of `BufferResponse`.
   
   I am not caring about the other messages because they are less frequent in running except `AddCredit` which might have the same frequency with `BufferResponse`. Although the size of `AddCredit` is very small, I am not sure whether it would still boost the netty memory overhead in practice.
   
   I noticed that the sender side also reuses the same new `ZeroCopyNettyMessageDecoder` with receiver side, and I have some concerns here. This new decoder would maintain/manage the overhead memory for frame and header parts compared with previous embedded `LengthFieldBasedFrameDecoder` in netty. If the new decoder is more efficient or memory saving than the `LengthFieldBasedFrameDecoder` (e.g. for the case of `AddCredit` mentioned above), it is reasonable, otherwise I do not prefer to reuse it on sender side for two reasons:
   
   - To do so, actually we do not unify the decoder on both sender and receiver side, because we also define different `NettyMessageParser` implementations inside `ZeroCopyNettyMessageDecoder`, which have the same effect with different decoders on sender and receiver sides.
   
   - It brings more changes and seems more complex. We can retain the previous `NettyMessageDecoder` for sender side and only removes the `BufferResponse` path from it, then we define a different decoder only for receiver side.
   
   Next I would further think how to refactor the implementation of `ZeroCopyNettyMessageDecoder`.
   
   

----------------------------------------------------------------
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