zhijiangW commented on a change in pull request #11687:
URL: https://github.com/apache/flink/pull/11687#discussion_r420186419
##########
File path:
flink-runtime/src/test/java/org/apache/flink/runtime/io/network/partition/consumer/RemoteInputChannelTest.java
##########
@@ -731,33 +726,29 @@ public void testFailureInNotifyBufferAvailable() throws
Exception {
buffer = checkNotNull(bufferPool.requestBuffer());
// trigger subscription to buffer pool
- failingRemoteIC.onSenderBacklog(1);
- successfulRemoteIC.onSenderBacklog(numExclusiveBuffers
+ 1);
- // recycling will call
RemoteInputChannel#notifyBufferAvailable() which will fail and
- // this exception will be swallowed and set as an error
in failingRemoteIC
+ channelWithoutPartition.onSenderBacklog(1);
+
channelWithPartition.onSenderBacklog(numExclusiveBuffers + 1);
+
+ // recycling will call
RemoteInputChannel#notifyBufferAvailable() which will not increase
+ // the unannounced credit if the channel has not
requested partition
buffer.recycleBuffer();
- buffer = null;
- try {
- failingRemoteIC.checkError();
- fail("The input channel should have an error
based on the failure in RemoteInputChannel#notifyBufferAvailable()");
- } catch (IOException e) {
- assertThat(e, hasProperty("cause",
isA(IllegalStateException.class)));
- }
Review comment:
The previous purpose was to verify that if one remote channel has not
requested partition, then the notify credit available will fail with
`IllegalStateException` because of non-initialized `PartitionRequestClient` in
`RemoteInputChannel`. I think this test is not meaningful in practice, because
if the partition request have not happened yet, then the `onSenderBacklog`
should also not happen as well. It seems not valid to explicitly call
`onSenderBacklog` to verify this logic without partition request.
My changes for this test is to verify that if the partition request have not
happened yet, then the announced credit would not be increased as a result
because this is only for the state recovery process to notify available
buffers, but this available buffer is not regarded as credit concept without
partition request. So I think it makes more sense than the previous test
purpose.
----------------------------------------------------------------
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]
