Github user NicoK commented on a diff in the pull request:

    https://github.com/apache/flink/pull/4509#discussion_r141673902
  
    --- Diff: 
flink-runtime/src/main/java/org/apache/flink/runtime/io/network/partition/consumer/RemoteInputChannel.java
 ---
    @@ -390,7 +390,63 @@ public BufferProvider getBufferProvider() throws 
IOException {
                return inputGate.getBufferProvider();
        }
     
    -   public void onBuffer(Buffer buffer, int sequenceNumber) {
    +   /**
    +    * Requests buffer from input channel directly for receiving network 
data.
    +    * It should always return an available buffer in credit-based mode.
    +    *
    +    * @return The available buffer.
    +    */
    +   public Buffer requestBuffer() {
    +           synchronized (availableBuffers) {
    +                   return availableBuffers.poll();
    +           }
    +   }
    +
    +   /**
    +    * Receives the backlog from producer's buffer response. If the number 
of available
    +    * buffers is less than the backlog length, it will request floating 
buffers from buffer
    +    * pool, and then notify unannounced credits to the producer.
    +    *
    +    * @param backlog The number of unsent buffers in the producer's sub 
partition.
    +    */
    +   private void onSenderBacklog(int backlog) {
    +           int numRequestedBuffers = 0;
    +
    +           synchronized (availableBuffers) {
    +                   // Important: the isReleased check should be inside the 
synchronized block.
    +                   if (!isReleased.get()) {
    +                           senderBacklog.set(backlog);
    +
    +                           while (senderBacklog.get() > 
availableBuffers.size() && !isWaitingForFloatingBuffers.get()) {
    --- End diff --
    
    By taking all buffers until none are available any more, we do not really 
ensure a fair distribution where large backlogs on one channel may actually 
"starve" another (except for the exclusive buffers). Floating buffers should be 
assigned round-robin among the channels.
    
    Also, actually, the total number of required buffers should be `backlog + 
initialCredit - currentCredit` with `currentCredit` being the credit that was 
guaranteed, i.e. `availableBuffers.size()`.
    
    Let me think about this a bit more to sort out the details - I'll come back 
to you.


---

Reply via email to