lindong28 commented on a change in pull request #15633:
URL: https://github.com/apache/flink/pull/15633#discussion_r614520175
##########
File path:
flink-test-utils-parent/flink-test-utils/src/main/java/org/apache/flink/networking/NetworkFailureHandler.java
##########
@@ -73,8 +76,10 @@ public NetworkFailureHandler(
/** Closes the specified channel after all queued write requests are
flushed. */
static void closeOnFlush(Channel channel) {
- if (channel.isConnected()) {
+ if (channel.isConnected() &&
!channelsBeingClosed.containsKey(channel)) {
+ channelsBeingClosed.put(channel, channel);
channel.write(ChannelBuffers.EMPTY_BUFFER).addListener(ChannelFutureListener.CLOSE);
+ channelsBeingClosed.remove(channel);
Review comment:
Thanks for the review @dawidwys!
Hmm.. I think the current approach is better because:
1) The code is less and also simpler to understand.
2) The approach makes less assumption in the sense that it just prevents
closeOnFlush() from being called recursively (which we know will lead to live
lock).
In comparison, the alternative approach assumes that closeOnFlush() will
never need to be called more than once for the same Channel), which may be true
but is (IMO) not that obvious.
Is there any concern that we allow closeOnFlush() to be called multiple
times for the same channel? If not, maybe it is better to take the simpler
approach.
--
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]
