You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ratis.apache.org by GitBox <gi...@apache.org> on 2020/05/01 03:36:32 UTC
[GitHub] [incubator-ratis] runzhiwang opened a new pull request #83: RATIS-901. Fix Failed UT: WatchRequestTests.testWatchRequestClientTimeout
runzhiwang opened a new pull request #83:
URL: https://github.com/apache/incubator-ratis/pull/83
Why WatchRequestTests.testWatchRequestClientTimeout failed.
The test try to watch a log index which will never be committed, so client can not receive reply, and connection closed throw AlreadyClosedException, trigger TimeoutIOException.
But when throw AlreadyClosedException, suggested new leader is null, because connection has been closed, so RaftClientImpl::handleIOException will [random select](https://github.com/apache/incubator-ratis/blob/master/ratis-client/src/main/java/org/apache/ratis/client/impl/RaftClientImpl.java#L418) a new leader, so it maybe sometimes throw NotLeaderException rather than TimeoutIOException.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-ratis] runzhiwang commented on a change in pull request #83: RATIS-901. Fix Failed UT: WatchRequestTests.testWatchRequestClientTimeout
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on a change in pull request #83:
URL: https://github.com/apache/incubator-ratis/pull/83#discussion_r419060759
##########
File path: ratis-server/src/test/java/org/apache/ratis/WatchRequestTests.java
##########
@@ -457,8 +460,13 @@ static void runTestWatchRequestClientTimeout(TestParameters p) throws Exception
ex.getCause().getClass());
if (ex.getCause() != null) {
if (ex.getCause().getCause() != null) {
- Assert.assertEquals(TimeoutIOException.class,
- ex.getCause().getCause().getClass());
+ // when client closed and throw TimeoutException,
+ // RaftClientImpl::handleIOException will random select a leader
+ // because suggested new leader is null.
Review comment:
@bshashikant Thanks for your review. I have updated.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-ratis] runzhiwang commented on a change in pull request #83: RATIS-901. Fix Failed UT: WatchRequestTests.testWatchRequestClientTimeout
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on a change in pull request #83:
URL: https://github.com/apache/incubator-ratis/pull/83#discussion_r420117611
##########
File path: ratis-server/src/test/java/org/apache/ratis/WatchRequestTests.java
##########
@@ -446,14 +445,18 @@ static void runTestWatchRequestClientTimeout(TestParameters p) throws Exception
final Logger LOG = p.log;
CompletableFuture<RaftClientReply> watchReply;
- watchReply = p.sendWatchRequest(1000);
+ // watch 1000 which will never be committed
+ // so client can not receive reply, and connection closed, throw TimeoutException.
+ // We should not retry, because if retry, RaftClientImpl::handleIOException will random select a leader,
+ // then sometimes throw NotLeaderException.
+ watchReply = p.sendWatchRequest(1000, RetryPolicies.noRetry());
try {
watchReply.get();
fail("runTestWatchRequestClientTimeout failed");
} catch (Exception ex) {
LOG.error("error occurred", ex);
- Assert.assertEquals(RaftRetryFailureException.class,
+ Assert.assertEquals(AlreadyClosedException.class,
Review comment:
@bshashikant Thanks for your review. I have updated.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-ratis] bshashikant commented on a change in pull request #83: RATIS-901. Fix Failed UT: WatchRequestTests.testWatchRequestClientTimeout
Posted by GitBox <gi...@apache.org>.
bshashikant commented on a change in pull request #83:
URL: https://github.com/apache/incubator-ratis/pull/83#discussion_r419512537
##########
File path: ratis-server/src/test/java/org/apache/ratis/WatchRequestTests.java
##########
@@ -446,14 +445,18 @@ static void runTestWatchRequestClientTimeout(TestParameters p) throws Exception
final Logger LOG = p.log;
CompletableFuture<RaftClientReply> watchReply;
- watchReply = p.sendWatchRequest(1000);
+ // watch 1000 which will never be committed
+ // so client can not receive reply, and connection closed, throw TimeoutException.
+ // We should not retry, because if retry, RaftClientImpl::handleIOException will random select a leader,
+ // then sometimes throw NotLeaderException.
+ watchReply = p.sendWatchRequest(1000, RetryPolicies.noRetry());
try {
watchReply.get();
fail("runTestWatchRequestClientTimeout failed");
} catch (Exception ex) {
LOG.error("error occurred", ex);
- Assert.assertEquals(RaftRetryFailureException.class,
+ Assert.assertEquals(AlreadyClosedException.class,
Review comment:
Potentially , we can see both RaftRetryFailureException or AlreadyClosedException depending on how tests are executed using the same MiniRaftCluster. Let's add an OR condition here having both RaftRetryFailure or AlreadyClosedException.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-ratis] bshashikant commented on a change in pull request #83: RATIS-901. Fix Failed UT: WatchRequestTests.testWatchRequestClientTimeout
Posted by GitBox <gi...@apache.org>.
bshashikant commented on a change in pull request #83:
URL: https://github.com/apache/incubator-ratis/pull/83#discussion_r419047623
##########
File path: ratis-server/src/test/java/org/apache/ratis/WatchRequestTests.java
##########
@@ -457,8 +460,13 @@ static void runTestWatchRequestClientTimeout(TestParameters p) throws Exception
ex.getCause().getClass());
if (ex.getCause() != null) {
if (ex.getCause().getCause() != null) {
- Assert.assertEquals(TimeoutIOException.class,
- ex.getCause().getCause().getClass());
+ // when client closed and throw TimeoutException,
+ // RaftClientImpl::handleIOException will random select a leader
+ // because suggested new leader is null.
Review comment:
I think probbaly, we should ensure there is no retry in the test by setting the attempt count to 1 in the retry policy.. In such case, it will always fail consistently with TimeoutException which is the goal here.
----------------------------------------------------------------
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:
users@infra.apache.org