[FLINK-7745][tests] add tests for ensuring NetworkBufferPool overprovisioning behaviour #4758
Conversation
…isioning behaviour
| int buffersToTakeFromPool2 = numBuffers - buffersToTakeFromPool1; | ||
|
|
||
| List<Buffer> buffers = new ArrayList<>(numBuffers); | ||
| BufferPool lbp1 = null, lbp2 = null; |
There was a problem hiding this comment.
Yes, after ~30 seconds I was able to decrypt this name, but please do not use abbreviations :)
| List<Buffer> buffers = new ArrayList<>(numBuffers); | ||
| BufferPool lbp1 = null, lbp2 = null; | ||
| try { | ||
| lbp1 = networkBufferPool.createBufferPool(buffersToTakeFromPool2, numBuffers); |
There was a problem hiding this comment.
lbp1 vs buffersToTakeFromPool2?
There was a problem hiding this comment.
that's correct - although I also had to think about this one more time, last time I looked at this commit: buffersToTakeFromPool2 is the minimum number of buffers reserved for this pool while buffersToTakeFromPool1 is the actual number of buffers we request from the pool - I could add a comment regarding this to the variable declaration
| assertEquals(buffersToTakeFromPool1, lbp1.bestEffortGetNumOfUsedBuffers()); | ||
| assertEquals(numBuffers, lbp1.getNumBuffers()); | ||
|
|
||
| // create a second pool which requires more than are freely available at the moment |
|
|
||
| Buffer buffer = lbp2.requestBuffer(); | ||
| buffers.add(buffer); | ||
| assertNotNull(buffer); |
There was a problem hiding this comment.
move this check one line above?
There was a problem hiding this comment.
right - also the same code some lines above (to be on the safe side if this ever fails the check)
| for (Buffer buffer : buffers) { | ||
| buffer.recycle(); | ||
| } | ||
| if (lbp1 != null) { |
There was a problem hiding this comment.
Maybe instead of this null checking use guava's Closer class?
There was a problem hiding this comment.
actually, we don't want to extend further/add dependencies on guava, afaik
There was a problem hiding this comment.
Shame :( Maybe we need our own Closer. It's a nice useful concept.
The testOverprovisioned() unit test wasn't really testing that a buffer returned from lbp1 is available to lbp2 afterwards, which is also fixed in addition to the improved comments in the tests.
|
Added some comments and also found out that the |
- rename occurences of `lbp` to more meaningful names
|
Thanks for your contribution @NicoK and the review @pnowojski. Merging this PR. |
…isioning behaviour [FLINK-7745][tests] address PR comments, fix test The testOverprovisioned() unit test wasn't really testing that a buffer returned from lbp1 is available to lbp2 afterwards, which is also fixed in addition to the improved comments in the tests. [FLINK-7745][network] address PR comments - rename occurences of `lbp` to more meaningful names This closes apache#4758.
What is the purpose of the change
Currently, there are no unit tests verifying
NetworkBufferPool's behaviour in the case that the available number of buffers is too small for it to create LocalBufferPool instances. This PR adds some.Brief change log
NetworkBufferPoolnot having as many buffers as requiredVerifying this change
This change only adds unit tests.
Does this pull request potentially affect one of the following parts:
@Public(Evolving): (no)Documentation