pnowojski 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_r226945997
##########
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);
Review comment:
This would be a longer story to fully explained. There are multiple problems
with mockito, some (most?) of them come from how it's being used:
1. Whenever you write `mock(ResultPartitionConsumableNotifier.class);`, you
basically define an empty, with default implementation of returning `nulls` and
ignoring method calls. If you use `mock(Something.class)` twice for the same
class, you duplicate such naive implementation. Mockito makes it so easy that
such declarations are duplicated many times around the code base. Now when
something changes between the interactions with the mocked class, you have to
fix it everywhere separately.
2. Default implementation that ignores method calls/returns null is
terrifying. Again, in case of refactoring/changing contract, this can result in
irrelevant false positive test failures that are sometimes hard to track-down.
For example if you add a new method `boolean isBlocked()` to the mocked
interface. All of the mocks are silently broken after this. With proper code,
after adding a method your code will not compile and it's also easy to find
missing implementation because ...
3. ... no Intellij support for refactoring/finding usages. Intellij doesn't
rapport mocked methods as "overloaded" for example not it doesn't show up when
you look for classes that are extending interface that you are mocking
4. Mockito tends to duplicate mock over and over again around the code
base.
With proper mocks, people are automatically trying to re-use them.
Or saying other way around. Would you accept in code review, if someone was
about to submit a real implementation of some class, that returns `null` for
half of the methods (while the real classes are not nullable), ignores silently
all of the current method calls (not only those that you intended to) and also
any future ones (think about this: `mock(List.class)` and passing it to
somewhere, that after refactoring will call `list.clear()`). And on top of all
of that, such class would be duplicated 10 times around the code base?
I don't want to press you to change other places in which Mockito was being
used extensively before, but generally speaking we are trying to avoid it and
we shouldn't be making things worse - new tests should avoid mockito and
whenever you touch something that's mockito tested, this might be a good reason
to get rid of such mocks.
----------------------------------------------------------------
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:
[email protected]
With regards,
Apache Git Services
