[ 
https://issues.apache.org/jira/browse/FLINK-8755?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16380019#comment-16380019
 ] 

ASF GitHub Bot commented on FLINK-8755:
---------------------------------------

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

    https://github.com/apache/flink/pull/5581#discussion_r171181261
  
    --- Diff: 
flink-runtime/src/test/java/org/apache/flink/runtime/io/network/partition/SubpartitionTestBase.java
 ---
    @@ -182,7 +182,7 @@ private static void assertNextBufferOrEvent(
                        ResultSubpartitionView readView,
                        int expectedReadableBufferSize,
                        boolean expectedIsBuffer,
    -                   Class<? extends AbstractEvent> expectedEventClass,
    +                   @Nullable Class<? extends AbstractEvent> 
expectedEventClass,
    --- End diff --
    
    I would prefer for this method to return `Buffer` and:
    ```
    void assertNextBuffer(...) {
      Buffer buffer = assertNextBufferOrEvent(...);
      assertTrue(buffer.isBuffer());
      buffer.recycleBuffer();
    }
    
    void assertNextEvent(...) {
      Buffer buffer = assertNextBufferOrEvent(...);
      assertFalse(buffer.isBuffer());
      assertThat(EventSerializer.fromBuffer(buffer, ...), ...);
      buffer.recycleBuffer();
    }
    ```
    
    btw, you are not recycling the buffer in case of failure


> SpilledSubpartitionView wrongly relys on the backlog for determining whether 
> more data is available
> ---------------------------------------------------------------------------------------------------
>
>                 Key: FLINK-8755
>                 URL: https://issues.apache.org/jira/browse/FLINK-8755
>             Project: Flink
>          Issue Type: Sub-task
>          Components: Network
>            Reporter: Nico Kruber
>            Assignee: Nico Kruber
>            Priority: Blocker
>             Fix For: 1.5.0
>
>
> {code}
> public BufferAndBacklog getNextBuffer() throws IOException, 
> InterruptedException {
> //...
>         int newBacklog = parent.decreaseBuffersInBacklog(current);
>         return new BufferAndBacklog(current, newBacklog > 0, newBacklog, 
> nextBufferIsEvent);
> {code}
> relies on the backlog to signal further data availability. However, if there 
> are only events left in the buffer queue, their buffers are not included in 
> the backlog counting and therefore, {{isMoreAvailable}} will be wrongly 
> {{false}} here.



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

Reply via email to