zhijiangW commented on a change in pull request #11687:
URL: https://github.com/apache/flink/pull/11687#discussion_r419502301
##########
File path:
flink-runtime/src/main/java/org/apache/flink/runtime/io/network/partition/consumer/LocalInputChannel.java
##########
@@ -87,10 +90,14 @@ public LocalInputChannel(
int maxBackoff,
InputChannelMetrics metrics) {
- super(inputGate, channelIndex, partitionId, initialBackoff,
maxBackoff, metrics.getNumBytesInLocalCounter(),
metrics.getNumBuffersInLocalCounter());
+ super(inputGate, channelIndex, partitionId, initialBackoff,
maxBackoff, metrics);
this.partitionManager = checkNotNull(partitionManager);
this.taskEventPublisher = checkNotNull(taskEventPublisher);
+ // In most cases we only need one buffer for reading recovered
state except for very large record.
+ // Then only one floating buffer is required. Even though we
need more buffers for recovery for
+ // large record, it only increases some interactions with pool.
+ this.bufferManager = new BufferManager(this, 1);
Review comment:
Yes, I agree it is a bit hard to understand.
The `numRequiredBuffers` factor is only the complex point of this buffer
manager model for getting a bit optimization. Actually we can give any initial
value for `numRequiredBuffers` (e.g. 0) for unifying the local and remote
channels. And ideally we should adjust this value based on how many total
channel states are under unspilling exactly like the concept of `backlog` in
credit-based mode.
Actually any value for `numRequiredBuffers` can work correctly now and the
only cost is increasing some unnecessary interactions between `BufferManager`
and `LocalBufferPool`.
I am really a bit torn here when implementation whether to retain this
optimization.
----------------------------------------------------------------
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]
