pnowojski commented on a change in pull request #7713: [FLINK-10995][network]
Copy intermediate serialization results only once for broadcast mode
URL: https://github.com/apache/flink/pull/7713#discussion_r262493971
##########
File path:
flink-runtime/src/test/java/org/apache/flink/runtime/io/network/api/writer/RecordWriterTest.java
##########
@@ -461,6 +572,56 @@ private void
emitRecordWithBroadcastPartitionerOrBroadcastEmitRecord(boolean isB
}
}
+ private void randomEmitMixedBroadcastBufferOrEvent(boolean
isBroadcastEvent) throws Exception {
Review comment:
I'm not sure if I understand the idea behind the unit tests in this file.
What's the point of asserting the count and the size of buffers? In doesn't
check for the correctness, nor it checks for the performance it just checks for
some private implementation detail.
For performance testing we have micro benchmarks.
For correctness we should only check that certain scenarios like, writing x,
y, z records or events and/or mixed with broadcasting
records/events/watermarks, result with the correct output content of the
buffers, regardless of the number/size of the buffers on the output. Otherwise
if we optimise/refactor something, all of those tests will start yielding false
positive errors.
I'm not sure if you understand my concern/agree with it. If yes, and If you
do not want to fix the old tests, could you please at least re-write the new
test? This `randomEmitMixedBroadcastBufferOrEvent` test shouldn't care for
number of `getNumberOfCreatedBuffers`.
----------------------------------------------------------------
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
