[GitHub] [flink] zhijiangW commented on a change in pull request #10492: [FLINK-15140][runtime] Fix shuffle data compression doesn't work with BroadcastRecordWriter.

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

zhijiangW commented on a change in pull request #10492: [FLINK-15140][runtime] Fix shuffle data compression doesn't work with BroadcastRecordWriter.
URL: https://github.com/apache/flink/pull/10492#discussion_r355306140
 
 

 ##########
 File path: flink-runtime/src/main/java/org/apache/flink/runtime/io/network/buffer/BufferConsumer.java
 ##########
 @@ -44,6 +44,9 @@
 
 	private int currentReaderPosition;
 
+	/** Whether this BufferConsumer is copied from another BufferConsumer instance. */
+	private final boolean isCopied;
 
 Review comment:
   TBH I do not like the way of introducing this property inside `BufferConsumer` for several concerns:
   
   1. This property is not the basic characters of buffer or orienting specific scenarios. And it is somehow orienting/relying on the implementation details.
   
   2. In essence this property is coupled with both broadcast improvement and compression limitation. In other words it is not easy to explain why one `BufferConsumer` would be copied by others to make others understand the background. Firstly we have to explain that it is caused by the specific broadcast improvement, and further we have to explain the current compression implementation has the limitation of reusing the original buffer to copy compressed results only for pipelined partition.
   
   Therefore I prefer to introducing the compression feature only for batch jobs ATM, and our initial motivation was also for reducing the disk IO cost. Although for streaming job the compression can also reduce the network bandwidth, it is meanwhile bringing the extra CPU cost and compression/decompression time cost. So for the streaming job without bottleneck of network bandwidth, it is not suggested to enable this feature.
   
   Most importantly the current compression implementation for streaming job brings more customize changes, both for this property in `BufferConsumer` and also the local channel property while getting next buffer from view. I guess we might have better solutions for streaming compression future. WDYT @pnowojski ?

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