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/04/27 07:50:08 UTC
[GitHub] [incubator-ratis] runzhiwang opened a new pull request #65: RATIS-881. Fix Failed unit test because test before MiniRaftCluster ready
runzhiwang opened a new pull request #65:
URL: https://github.com/apache/incubator-ratis/pull/65
**What's the probelm ?**
**What's the reason ?**
[](url)
[RaftServerMetrics::getPeerCommitIndexGauge](https://github.com/apache/incubator-ratis/blob/master/ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerMetrics.java#L141) happens before [RaftServerMetrics::addPeerCommitIndexGauge](https://github.com/apache/incubator-ratis/blob/master/ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerMetrics.java#L122).
When some RaftServerImpl [setRole(RaftPeerRole.LEADER, "changeToLeader")](https://github.com/apache/incubator-ratis/blob/master/ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerImpl.java#L345), the statement [waitForLeader](https://github.com/apache/incubator-ratis/blob/master/ratis-server/src/test/java/org/apache/ratis/RaftBasicTests.java#L446) succ to get leader and test begin, but [role.startLeaderState](https://github.com/apache/incubator-ratis/blob/master/ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerImpl.java#L349) ->
[new LeaderState](https://github.com/apache/incubator-ratis/blob/master/ratis-server/src/main/java/org/apache/ratis/server/impl/RoleInfo.java#L94) ->
[LeaderState::addSenders](https://github.com/apache/incubator-ratis/blob/master/ratis-server/src/main/java/org/apache/ratis/server/impl/LeaderState.java#L409)->[RaftServerMetrics::addFollower](https://github.com/apache/incubator-ratis/blob/master/ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerMetrics.java#L106) -> [RaftServerMetrics::addPeerCommitIndexGauge](https://github.com/apache/incubator-ratis/blob/master/ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerMetrics.java#L122) has not finished.
----------------------------------------------------------------
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] lokeshj1703 commented on pull request #65: RATIS-881. Fix Failed unit test because test before MiniRaftCluster ready
Posted by GitBox <gi...@apache.org>.
lokeshj1703 commented on pull request #65:
URL: https://github.com/apache/incubator-ratis/pull/65#issuecomment-620388236
@runzhiwang Thanks for the contribution! @bshashikant Thanks for the review! I have committed the PR to master branch.
Made a minor change in the commit: Removed @VisibleForTesting annotation on RaftServerImpl@isLeaderReady.
----------------------------------------------------------------
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] lokeshj1703 edited a comment on pull request #65: RATIS-881. Fix Failed unit test because test before MiniRaftCluster ready
Posted by GitBox <gi...@apache.org>.
lokeshj1703 edited a comment on pull request #65:
URL: https://github.com/apache/incubator-ratis/pull/65#issuecomment-620388236
@runzhiwang Thanks for the contribution! @bshashikant Thanks for the review! I have committed the PR to master branch.
Made a minor change in the commit made to master: Removed @VisibleForTesting annotation on RaftServerImpl@isLeaderReady.
----------------------------------------------------------------
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] lokeshj1703 commented on a change in pull request #65: RATIS-881. Fix Failed unit test because test before MiniRaftCluster ready
Posted by GitBox <gi...@apache.org>.
lokeshj1703 commented on a change in pull request #65:
URL: https://github.com/apache/incubator-ratis/pull/65#discussion_r415703916
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerImpl.java
##########
@@ -460,6 +460,20 @@ public String toString() {
return null;
}
+ @VisibleForTesting
+ public boolean isLeaderReady() {
+ if (!isLeader()) {
+ return false;
+ }
+
+ final LeaderState leaderState = role.getLeaderState().orElse(null);
+ if (leaderState == null || !leaderState.isReady()) {
+ return false;
+ }
+
+ return true;
Review comment:
We can simplify this as.
`return leaderState != null && leaderState.isReady();`
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerImpl.java
##########
@@ -460,6 +460,20 @@ public String toString() {
return null;
}
+ @VisibleForTesting
+ public boolean isLeaderReady() {
+ if (!isLeader()) {
Review comment:
We can remove this check as leaderState == null covers the case and use this function in checkLeaderState line 451 as well.
----------------------------------------------------------------
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 pull request #65: RATIS-881. Fix Failed unit test because test before MiniRaftCluster ready
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on pull request #65:
URL: https://github.com/apache/incubator-ratis/pull/65#issuecomment-619946904
@lokeshj1703 Thanks for your review. I have updated the patch.
----------------------------------------------------------------
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