[GitHub] [flink] pnowojski commented on a change in pull request #17573: [FLINK-24190][runtime] Forbid to split the first record in the buffer if it physically fit it.

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

pnowojski commented on a change in pull request #17573:
URL: https://github.com/apache/flink/pull/17573#discussion_r737460618



##########
File path: flink-runtime/src/main/java/org/apache/flink/runtime/io/network/partition/BufferWritingResultPartition.java
##########
@@ -327,12 +331,13 @@ private BufferBuilder appendBroadcastDataForRecordContinuation(
         // with a complete record.
         // !! The next two lines can not change order.
         final int partialRecordBytes = buffer.appendAndCommit(remainingRecordBytes);
-        createBroadcastBufferConsumers(buffer, partialRecordBytes);
+        createBroadcastBufferConsumers(buffer, partialRecordBytes, 0);

Review comment:
       ditto

##########
File path: flink-runtime/src/main/java/org/apache/flink/runtime/io/network/partition/BufferWritingResultPartition.java
##########
@@ -298,7 +302,7 @@ private BufferBuilder appendUnicastDataForRecordContinuation(
         // with a complete record.
         // !! The next two lines can not change order.
         final int partialRecordBytes = buffer.appendAndCommit(remainingRecordBytes);
-        addToSubpartition(buffer, targetSubpartition, partialRecordBytes);
+        addToSubpartition(buffer, targetSubpartition, partialRecordBytes, 0);

Review comment:
       why not `addToSubpartition(buffer, targetSubpartition, partialRecordBytes, partialRecordBytes);`? (and if so add a test coverage)

##########
File path: flink-runtime/src/main/java/org/apache/flink/runtime/io/network/partition/consumer/LocalInputChannel.java
##########
@@ -346,7 +346,8 @@ void announceBufferSize(int newBufferSize) {
 
     @Override
     int getBuffersInUseCount() {
-        return subpartitionView.getNumberOfQueuedBuffers();
+        ResultSubpartitionView view = subpartitionView;
+        return view == null ? 0 : view.getNumberOfQueuedBuffers();

Review comment:
       1. is this change related to this ticket? How did you find this issue?
   2. it sounds like this might deserve an independent bug ticket and might has to be backported to 1.14.x.
   3. test coverage?




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

To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org