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 very 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:
[email protected]
With regards,
Apache Git Services
