-
Notifications
You must be signed in to change notification settings - Fork 3.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HBASE-26679 Wait on the future returned by FanOutOneBlockAsyncDFSOutp… #4039
Conversation
…ut.flush would stuck
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the fix is correct.
And on the tests, I think we could just use create a FanOutOneBlockAsyncDFSOutput using constructor, and use Mockito to fake all the out going instances such as DFSClient, DistributedFileSystem, ClientProtocol, Channel, etc, then we can fully control how to simulate slow DNs?
...e-asyncfs/src/main/java/org/apache/hadoop/hbase/io/asyncfs/FanOutOneBlockAsyncDFSOutput.java
Show resolved
Hide resolved
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
@comnetwork PTAL. The trick here is to add a ChannelInboundHandler to eat all the responses to simulate a slow DN. So we only need to get the datanodeInfoMap to implement the UT. Without the fix here the UT will fail with TimeoutException. You could try it locally to see if it works for you. Thanks. |
@Apache9 , thank you very much, I have already modify my test code following your patch. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No big problems.
The only concern is to increase the READER_TIMEOUT value for this test, but not a blocker. If you insist this is not important for now, I'm also OK.
} | ||
}); | ||
assertTrue(protobufDecoderNames.size() == 1); | ||
dn1Channel.pipeline().addAfter(protobufDecoderNames.get(0), "dn1AckReceivedHandler", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good.
* </pre> | ||
*/ | ||
@Test | ||
public void testFlushStuckWhenOneDataNodeFailedBeforeOtherDataNodeAck() throws Exception { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, seems no big problem if we just ignore all inbound messages.
But it needs extra consideration, better not introduce more conditions to a test. When I first tried to implement the UT, I used a boolean flag to drop exact one package and the heartbeat message messed up the test. In the future if we want to change the implementation to simulate more, maybe another developer will fall into this too and waste more time...
@Apache9, ok, I have already introduce a separated test class |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
The javac warning is a FutureReturnValueIgnored in test code, not very important. Let me merge. Thanks @comnetwork for the great finding and fixing! |
…ut.flush would stuck (#4039) Signed-off-by: Duo Zhang <zhangduo@apache.org>
…ut.flush would stuck (#4039) Signed-off-by: Duo Zhang <zhangduo@apache.org>
…ut.flush would stuck