zhijiangW commented on pull request #13523: URL: https://github.com/apache/flink/pull/13523#issuecomment-718688119
Thanks for the further confirmation @StephanEwen ! > Do we need to FileRegionResponse to extend DefaultFileRegion, or can the FileRegionResponse contain a DefaultFileRegion object? Given the complex interface of FileRegion in Netty (and how it may change between versions) might be good to avoid inheriting from it. As we confirmed before, we only need one `BufferResponse` message for both `Buffer` and `FileRegion` ways. I tried to introduce `FileRegionBuffer` to implement `Buffer` interface and it has the `FileRegion` inside. Then we do not need to touch any previous `Buffer` instances and compatible with relevant existing interfaces. But we can not reuse `DefaultFileRegion` provided by netty directly, since the `DefaultFileRegion#deallocate()` would close underlying `FileChannel` for every transfer. So we have to extend `DefaultFileRegion` to override `#deallocate` method with empty operation. > You are right about buffer#recycleBuffer, buffer#setAllocator and BufferResponse#getBuffer - we need to handle this differently for buffers and regions. I guess we do not need consider these issues now, as I assume `FileRegionBuffer` implement `Buffer` interface compatible with existing methods. Also we do not need any `Object` field in `BufferResponse` component. > Handling SSL Totally agree with your points for it. FYI: My new version codes are almost ready to resolve all the above comments. I will update this PR probably early tomorrow after polishing the code a bit. ---------------------------------------------------------------- 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]
