zhijiangW commented on pull request #13523: URL: https://github.com/apache/flink/pull/13523#issuecomment-717095708
Thanks for your pretty suggestive reviews @StephanEwen and I really get some profound, original insights from you! > Compatibility with SSL I really overlooked the SSL branch before. After double verify, the file region way is indeed not compatible with SSL as you concerned, also mentioned in netty [issue](https://github.com/netty/netty/issues/2075). ATM I do not think of a proper way to support SSL with file region. Then my rough thought is to retain two branches to work around. The previous `FileBufferReader` implementation with memory overhead is still valid if SSL enabled. And I introduce another implementation of `BoundedData.Reader` for taking file region way without SSL option. I guess SSL not widely used in practice, so most of the scenarios can still benefit from this improvement of memory overhead. WDYT or have other better options? > Naming I ever a bit torn whether to retain the previous naming for the reason you mentioned above. I totally agree with your saying `thinking of a file region as a lazy buffer`, then +1 to keep the previous naming. > An slightly different structure for Netty Messages After double checking, I had the misunderstanding of the netty stack call before to result in current implementation. I wrongly thought that `NettyMessage#write` can only write `FileRegion` instance without any `ByteBuf` instance in order to guarantee the followup `FileRegion#transferTo` called properly. So I only allocated header buffer in `NettyMessage#write` and write it together with `FileRegion#transferTo`, that is the previous consideration without reusing `DefaultFileRegion` directly. Your above inputs of refactoring `NettyMessage#write` give me some insights to try out again and dispel my above misunderstanding. We can indeed write header ByteBuf to `ChannelOutboundInvoker` directly for both `BufferResponse` and `FileRegion` ways, then the code path is almost the same for them now. I conclude the following changes: - Introduce another `FileRegionResponse` which extends `DefaultFileRegion`, because we need to override the `#deallocate` method not to close file channel for every call. - Refactor current `NettyMessage#write` method into `write(ChannelOutboundInvoker out, ByteBufAllocator allocator, ChannelPromise promise)`, then return `void` for it. I find it will also bring some logics refactoring in some unit tests. - Make `BufferResponse#buffer` as an object which can be either `Buffer` or `FileRegionResponse`, but we also need to consider how to refactor current usages like `buffer#recycleBuffer`, `buffer#setAllocator` and `BufferResponse#getBuffer` after `buffer` becoming an object. - Keep `NettyMessage` still as abstract class since we do not need to introduce extra `NettyMessage` subclass to reuse existing `BufferResponse` as above. ---------------------------------------------------------------- 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]
