You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ratis.apache.org by GitBox <gi...@apache.org> on 2021/04/21 09:23:56 UTC
[GitHub] [ratis] softgitron opened a new pull request #465: Ratis 1362 Intermittent NPE in RaftReconfigurationBaseTest
softgitron opened a new pull request #465:
URL: https://github.com/apache/ratis/pull/465
## What changes were proposed in this pull request?
Fix `org.apache.ratis.grpc.TestRaftReconfigurationWithGrpc` testcase using following methods:
- Changed hard coded sleep time to poll based sleep to make sure that there is enough time on every machine
- Added poll based sleep before test case assertions to make sure, that everything is run properly before evaluation of the results.
Was part of the https://github.com/apache/ratis/pull/461
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/RATIS-1362
## How was this patch tested?
This is a fix for a automated test case, so no particularly special testing have been made. Fix has been tested manually using mvn test and Ratis CI.
--
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] [ratis] szetszwo commented on a change in pull request #465: Ratis 1362 Intermittent NPE in RaftReconfigurationBaseTest
Posted by GitBox <gi...@apache.org>.
szetszwo commented on a change in pull request #465:
URL: https://github.com/apache/ratis/pull/465#discussion_r620098916
##########
File path: ratis-server/src/test/java/org/apache/ratis/server/impl/RaftReconfigurationBaseTest.java
##########
@@ -340,7 +337,7 @@ void runTestBootstrapReconf(int numNewPeer, boolean startNewPeer, CLUSTER cluste
LOG.info("Start changing the configuration: {}",
asList(c1.allPeersInNewConf));
final AtomicReference<Boolean> success = new AtomicReference<>();
-
+
Review comment:
```suggestion
```
Will revert his whitespace change before merging.
--
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] [ratis] softgitron commented on pull request #465: Ratis 1362 Intermittent NPE in RaftReconfigurationBaseTest
Posted by GitBox <gi...@apache.org>.
softgitron commented on pull request #465:
URL: https://github.com/apache/ratis/pull/465#issuecomment-826370841
> @softgitron , TestRaftReconfigurationWithSimulatedRpc failed in the check. Could you take a look?
For me it looks like another flaky testcase. Failed test case should not be related to this change though. According to IntelliJ's "Find Usages" functionality runTestBootstrapReconf function is used only by "testBootstrapReconfWithSingleNodeAddOne", "testBootstrapReconfWithSingleNodeAddTwo" and "testBootstrapReconf" cases.
In addition all test cases went successfully through in personal CI: https://github.com/apache/ratis/actions/runs/783297327
--
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] [ratis] szetszwo commented on a change in pull request #465: Ratis 1362 Intermittent NPE in RaftReconfigurationBaseTest
Posted by GitBox <gi...@apache.org>.
szetszwo commented on a change in pull request #465:
URL: https://github.com/apache/ratis/pull/465#discussion_r619782326
##########
File path: ratis-server/src/test/java/org/apache/ratis/server/impl/RaftReconfigurationBaseTest.java
##########
@@ -339,8 +336,8 @@ void runTestBootstrapReconf(int numNewPeer, boolean startNewPeer, CLUSTER cluste
final PeerChanges c1 = cluster.addNewPeers(numNewPeer, startNewPeer);
LOG.info("Start changing the configuration: {}",
asList(c1.allPeersInNewConf));
- final AtomicReference<Boolean> success = new AtomicReference<>();
+ final AtomicReference<Boolean> success = new AtomicReference<>();
Review comment:
```suggestion
final AtomicReference<Boolean> success = new AtomicReference<>();
```
Let's revert the whitespace change.
--
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] [ratis] szetszwo commented on a change in pull request #465: Ratis 1362 Intermittent NPE in RaftReconfigurationBaseTest
Posted by GitBox <gi...@apache.org>.
szetszwo commented on a change in pull request #465:
URL: https://github.com/apache/ratis/pull/465#discussion_r619782326
##########
File path: ratis-server/src/test/java/org/apache/ratis/server/impl/RaftReconfigurationBaseTest.java
##########
@@ -339,8 +336,8 @@ void runTestBootstrapReconf(int numNewPeer, boolean startNewPeer, CLUSTER cluste
final PeerChanges c1 = cluster.addNewPeers(numNewPeer, startNewPeer);
LOG.info("Start changing the configuration: {}",
asList(c1.allPeersInNewConf));
- final AtomicReference<Boolean> success = new AtomicReference<>();
+ final AtomicReference<Boolean> success = new AtomicReference<>();
Review comment:
```suggestion
final AtomicReference<Boolean> success = new AtomicReference<>();
```
Let's revert the whitespace change.
--
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] [ratis] szetszwo commented on pull request #465: Ratis 1362 Intermittent NPE in RaftReconfigurationBaseTest
Posted by GitBox <gi...@apache.org>.
szetszwo commented on pull request #465:
URL: https://github.com/apache/ratis/pull/465#issuecomment-826291830
@softgitron , TestRaftReconfigurationWithSimulatedRpc failed in the check. Could you take a look?
--
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] [ratis] szetszwo merged pull request #465: RATIS-1362. Intermittent NPE in RaftReconfigurationBaseTest.
Posted by GitBox <gi...@apache.org>.
szetszwo merged pull request #465:
URL: https://github.com/apache/ratis/pull/465
--
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] [ratis] szetszwo commented on pull request #465: Ratis 1362 Intermittent NPE in RaftReconfigurationBaseTest
Posted by GitBox <gi...@apache.org>.
szetszwo commented on pull request #465:
URL: https://github.com/apache/ratis/pull/465#issuecomment-826292326
@softgitron , please use standard format for the title and the commit message for this and other pull requests; see https://github.com/apache/ratis/pull/466#issuecomment-826290632 . Thanks a lot.
--
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] [ratis] szetszwo commented on a change in pull request #465: Ratis 1362 Intermittent NPE in RaftReconfigurationBaseTest
Posted by GitBox <gi...@apache.org>.
szetszwo commented on a change in pull request #465:
URL: https://github.com/apache/ratis/pull/465#discussion_r619134541
##########
File path: ratis-server/src/test/java/org/apache/ratis/server/impl/RaftReconfigurationBaseTest.java
##########
@@ -339,17 +336,6 @@ void runTestBootstrapReconf(int numNewPeer, boolean startNewPeer, CLUSTER cluste
final PeerChanges c1 = cluster.addNewPeers(numNewPeer, startNewPeer);
LOG.info("Start changing the configuration: {}",
asList(c1.allPeersInNewConf));
- final AtomicReference<Boolean> success = new AtomicReference<>();
-
- Thread clientThread = new Thread(() -> {
- try {
- RaftClientReply reply = client.admin().setConfiguration(c1.allPeersInNewConf);
- success.set(reply.isSuccess());
- } catch (IOException ioe) {
- LOG.error("FAILED", ioe);
- }
- });
- clientThread.start();
Review comment:
The client should be started before "startNewPeer" below. Otherwise, the test is changed.
--
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] [ratis] softgitron commented on a change in pull request #465: Ratis 1362 Intermittent NPE in RaftReconfigurationBaseTest
Posted by GitBox <gi...@apache.org>.
softgitron commented on a change in pull request #465:
URL: https://github.com/apache/ratis/pull/465#discussion_r619638760
##########
File path: ratis-server/src/test/java/org/apache/ratis/server/impl/RaftReconfigurationBaseTest.java
##########
@@ -339,17 +336,6 @@ void runTestBootstrapReconf(int numNewPeer, boolean startNewPeer, CLUSTER cluste
final PeerChanges c1 = cluster.addNewPeers(numNewPeer, startNewPeer);
LOG.info("Start changing the configuration: {}",
asList(c1.allPeersInNewConf));
- final AtomicReference<Boolean> success = new AtomicReference<>();
-
- Thread clientThread = new Thread(() -> {
- try {
- RaftClientReply reply = client.admin().setConfiguration(c1.allPeersInNewConf);
- success.set(reply.isSuccess());
- } catch (IOException ioe) {
- LOG.error("FAILED", ioe);
- }
- });
- clientThread.start();
Review comment:
I see, it took me some time to understand this case correctly, but I think I got it now. My most recent commit restores execution order, but at the same time also fixes concurrency problems.
--
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] [ratis] softgitron commented on pull request #465: Ratis 1362 Intermittent NPE in RaftReconfigurationBaseTest
Posted by GitBox <gi...@apache.org>.
softgitron commented on pull request #465:
URL: https://github.com/apache/ratis/pull/465#issuecomment-826370841
> @softgitron , TestRaftReconfigurationWithSimulatedRpc failed in the check. Could you take a look?
For me it looks like another flaky testcase. Failed test case should not be related to this change though. According to IntelliJ's "Find Usages" functionality runTestBootstrapReconf function is used only by "testBootstrapReconfWithSingleNodeAddOne", "testBootstrapReconfWithSingleNodeAddTwo" and "testBootstrapReconf" cases.
In addition all test cases went successfully through in personal CI: https://github.com/apache/ratis/actions/runs/783297327
--
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] [ratis] softgitron commented on a change in pull request #465: Ratis 1362 Intermittent NPE in RaftReconfigurationBaseTest
Posted by GitBox <gi...@apache.org>.
softgitron commented on a change in pull request #465:
URL: https://github.com/apache/ratis/pull/465#discussion_r619638760
##########
File path: ratis-server/src/test/java/org/apache/ratis/server/impl/RaftReconfigurationBaseTest.java
##########
@@ -339,17 +336,6 @@ void runTestBootstrapReconf(int numNewPeer, boolean startNewPeer, CLUSTER cluste
final PeerChanges c1 = cluster.addNewPeers(numNewPeer, startNewPeer);
LOG.info("Start changing the configuration: {}",
asList(c1.allPeersInNewConf));
- final AtomicReference<Boolean> success = new AtomicReference<>();
-
- Thread clientThread = new Thread(() -> {
- try {
- RaftClientReply reply = client.admin().setConfiguration(c1.allPeersInNewConf);
- success.set(reply.isSuccess());
- } catch (IOException ioe) {
- LOG.error("FAILED", ioe);
- }
- });
- clientThread.start();
Review comment:
I see, it took me some time to understand this case correctly, but I think I got it now. My most recent commit restores execution order, but at the same time also fixes concurrency problems.
--
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] [ratis] szetszwo commented on pull request #465: Ratis 1362 Intermittent NPE in RaftReconfigurationBaseTest
Posted by GitBox <gi...@apache.org>.
szetszwo commented on pull request #465:
URL: https://github.com/apache/ratis/pull/465#issuecomment-826471583
@softgitron , thanks for checking. Let's re-run it.
--
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] [ratis] szetszwo commented on pull request #465: Ratis 1362 Intermittent NPE in RaftReconfigurationBaseTest
Posted by GitBox <gi...@apache.org>.
szetszwo commented on pull request #465:
URL: https://github.com/apache/ratis/pull/465#issuecomment-826291830
--
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