[GitHub] [flink] StephanEwen commented on pull request #13523: [FLINK-15981][network] Implement FileRegion way to shuffle file-based blocking partition in network stack

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

StephanEwen commented on pull request #13523:
URL: https://github.com/apache/flink/pull/13523#issuecomment-717133357


   Thanks, @zhijiangW - some thoughts on your suggestions here:
   
   > Introduce another FileRegionResponse which extends DefaultFileRegion, because we need to override the #deallocate method not to close file channel for every call.
   
   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.
   
   You are right about `buffer#recycleBuffer`, `buffer#setAllocator` and `BufferResponse#getBuffer` - we need to handle this differently for buffers and regions.
   
   > Handling SSL
   
   I think SSL is quite important, many setups need it for data security compliance reasons. So we definitely need to support it.
   For the time being it is probably okay to say that the optimization from this PR here is not active when SSL is active, that we keep using the original version with the higher memory use when SSL is active. Then we see how the sort-based shuffle develops, maybe we need to encourage people to use that one when SSL is active.
   


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