zhijiangW commented on a change in pull request #6809: [FLINK-10491][network]
Pass BufferPoolOwner in the constructor of LocalBufferPool
URL: https://github.com/apache/flink/pull/6809#discussion_r226525858
##########
File path:
flink-runtime/src/test/java/org/apache/flink/runtime/io/network/partition/ResultPartitionTest.java
##########
@@ -205,6 +216,66 @@ protected void testAddOnPartition(final
ResultPartitionType pipelined)
}
}
+ @Test
+ public void testReleaseMemoryOnBlockingPartition() throws Exception {
+ testReleaseMemory(ResultPartitionType.BLOCKING);
+ }
+
+ @Test
+ public void testReleaseMemoryOnPipelinedPartition() throws Exception {
+ testReleaseMemory(ResultPartitionType.PIPELINED);
+ }
+
+ /**
+ * Tests {@link ResultPartition#releaseMemory(int)} on a working
partition.
+ *
+ * @param resultPartitionType the result partition type to set up
+ */
+ private void testReleaseMemory(final ResultPartitionType
resultPartitionType) throws Exception {
+ final int numBuffers = 10;
+ final NetworkEnvironment network = new NetworkEnvironment(
+ new NetworkBufferPool(numBuffers, 128),
+ new LocalConnectionManager(),
+ new ResultPartitionManager(),
+ new TaskEventDispatcher(),
+ new KvStateRegistry(),
+ null,
+ null,
+ IOManager.IOMode.SYNC,
+ 0,
+ 0,
+ 2,
+ 8,
+ true);
+ final ResultPartitionConsumableNotifier notifier =
mock(ResultPartitionConsumableNotifier.class);
+ final ResultPartition resultPartition =
spy(createPartition(notifier, resultPartitionType, false));
Review comment:
I also considered the `spy` usage here may bring some concerns and thought
of the way you mentioned to avoid the `spy`. I should explain the reasons why
using `spy` finally.
I think there should have two separate tests for verifying different logics
in two dimessions:
1. Whether the `ResultPartition` is assigned as `BufferPoolOwner` correctly
during creating `BufferPool` for different types, and this test is for
verifying this relationship.
2. Verify the logic of interface implementation for
`BufferPoolOwner#releaseMemory` which is also missing currently, and I
mentioned this in previous comments. But I think it is not the scope of this PR
and I am willing to open a JIRA for it. At the beginning I also think the first
verify actually does not belong to the scope of this PR, just because of the
careless of migrating history codes to find history missing tests.
So if we want to check the state of `NetworkBufferPool` to avoid `spy` here,
we have to touch the detail logic of `BufferPoolOwner#releaseMemory` mentioned
in second part. And the first part is enough for this PR to avoid the mistake
of assigning `BufferPoolOwner`. What do you think?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
