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

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

zhijiangW edited a comment 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-584510940
 
 
   Thanks for the update @gaoyunhaii !
   
   While reviewing the core codes of `ZeroCopyNettyMessageDecoder` and `NettyMessageParser`, I have several concerns:
   
   1. It is hard to extend a new message type future, because it has to understand the implementation detail of above decoder and parser for properly judging this new message in different internal processes. It is better to make the decoder implementation detail transparent  for new message extension.
   
   2. There are many interactions among decoder, parser and `BufferResponse`, and it brings many intermediate states to maintain inside decoder and parser. It would make things more complex to understand and easy to cause potential bugs. E.g. The specific message header length is got from `NettyMessage` via interaction with parser and the intermediate state is maintained inside decoder.
   
   3. Actually the real decode work is mainly done by specific `NettyMessage#readFrom`, but now the decoder also involves in partial work which seems fragmented and not unified.
   
   Another possible options is to make `ZeroCopyNettyMessageDecoder` more light-weight and remove `NettyMessageParser`. We can delegate all the decode work to respective `NettyMessage`, and the current `ZeroCopyNettyMessageDecoder` can only handle the unified frame header for all the messages as before. We can refactor the current `NettyMessage#readFrom` method to define an abstract `decode` method instead. In order not to effect the existing messages on sender side, we can define a new `NettyClientMessage` to extend `NettyMessage` only for receiver side.
   
   The new defined `NettyClientMessage#decode` method takes the similar role as `RecordDeserializer`, it is aware of the implementation detail to parse the header and body properly, and return the proposed `DecodeResult` to indicate whether the header is fulfilled or the body is fulfilled or the received `ByteBuf` is fully consumed, etc.
   
   To do so, the `ZeroCopyNettyMessageDecoder` does not need to know the details of different `NettyMessage` instances and it only controls the general logic when to fire the full message to next handler. And if extending a new type of message future, it only needs to implement the abstract `decode` method for parsing itself, not aware of the upper `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