You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zookeeper.apache.org by hanm <gi...@git.apache.org> on 2017/07/24 00:29:07 UTC
[GitHub] zookeeper pull request #315: ZOOKEEPER-2577: Fix flaky testDuringLeaderSync ...
GitHub user hanm opened a pull request:
https://github.com/apache/zookeeper/pull/315
ZOOKEEPER-2577: Fix flaky testDuringLeaderSync test.
testDuringLeaderSync uses the presence of intermediate zoo.cfg.dynamic file to decide if the reconfig operation was succeeded or not. This is not a problem and is logically correct, however in tests that provisions QuorumPeer directly through MainThread, the configFile will be null and the resulted intermediate dynamic reconfig file will has a name of "null.cfg.dynamic". This causes problem because multiple test cases use MainThread to provision QuorumPeer so these tests will share a single "null.cfg.dynamic" file, as opposed to the zoo.cfg.dynamic file in their specific test folder when configFile was not null. Since we support running concurrent ant unit tests in apache build infrastructure, it is highly likely that multiple tests that rely on this shared null.cfg.dynamic file will execute simultaneously which create all sorts of failure cases (this also explains why if one tries to reproduce the test failures in a standalone fashion the test will always pass, with the absence
of the file sharing.).
This is problematic in multiple test cases and in particular cause this testDuringLeaderSync fail fairly frequently. There are multiple solutions to this problem:
* Implement retry with timeout logic when detecting the presence of intermediate config files in testDuringLeaderSync. This will fix this specific test but the fix is non-deterministic and other tests might still fail because of sharing of null.cfg.dynamic file.
* Always make sure to to feed a real config file when provision QuorumPeer. This however is an overkill as some cases a pure artificial QuorumPeer w/o real config file fit the use case well.
* The approach used in this patch: when generating intermediate config files if configFile is null, generate a random unique file name. This code path is supposed to only active when executing unit tests.
Testing done: running this patch on apache jenkins for the past week with 500 runs. Not only this test is fixed but overall stability of entirely unit tests are improved.
Please review @shralex @arshadmohammad
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/hanm/zookeeper working
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/zookeeper/pull/315.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #315
----
commit f3b813924323a5fb64f9242303f3f5dd40d91a83
Author: Michael Han <ha...@apache.org>
Date: 2017-07-23T06:03:46Z
ZOOKEEPER-2577: Fix flaky testDuringLeaderSync test caused by null config file name.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #315: ZOOKEEPER-2577: Fix flaky testDuringLeaderSync ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/zookeeper/pull/315
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #315: ZOOKEEPER-2577: Fix flaky testDuringLeaderSync ...
Posted by shralex <gi...@git.apache.org>.
Github user shralex commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/315#discussion_r129148693
--- Diff: src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java ---
@@ -1451,7 +1452,12 @@ public synchronized void restartLeaderElection(QuorumVerifier qvOLD, QuorumVerif
}
public String getNextDynamicConfigFilename() {
- return configFilename + QuorumPeerConfig.nextDynamicConfigFileSuffix;
+ if (configFilename != null) {
--- End diff --
A few comments:
- It's be better if configFileName is never null. But that may be a bigger change that we don't want to do here
- If its null, it may be better to return null here and avoid calling writeDynamicConfig.
- The calling test should either initiate configFileName properly or not rely on its existance to check reconfig completion. You can do the latter by looking at the quorumVerified directly.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #315: ZOOKEEPER-2577: Fix flaky testDuringLeaderSync ...
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/315#discussion_r129956031
--- Diff: src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java ---
@@ -1451,7 +1452,12 @@ public synchronized void restartLeaderElection(QuorumVerifier qvOLD, QuorumVerif
}
public String getNextDynamicConfigFilename() {
- return configFilename + QuorumPeerConfig.nextDynamicConfigFileSuffix;
+ if (configFilename != null) {
--- End diff --
Update pull request based on offline chat with Alex. The idea is to detect the null cases for configFileName and for those cases don't write intermediate reconfig file to disk because if we do, the shared "null.dynamic.cfg" file will be overwritten and might likely break other things without notice.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #315: ZOOKEEPER-2577: Fix flaky testDuringLeaderSync ...
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/315#discussion_r129727799
--- Diff: src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java ---
@@ -1451,7 +1452,12 @@ public synchronized void restartLeaderElection(QuorumVerifier qvOLD, QuorumVerif
}
public String getNextDynamicConfigFilename() {
- return configFilename + QuorumPeerConfig.nextDynamicConfigFileSuffix;
+ if (configFilename != null) {
--- End diff --
@shralex Turns out pretty trivial to guarantee a real configFileName in this case - update with one line change. Tested on apache jenkins the issue is fixed.
Note we still have configFileName as null for some other tests (such as FLE tests) but in those tests the intermediate reconfig file is not relevant so I'll just leave as they are.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #315: ZOOKEEPER-2577: Fix flaky testDuringLeaderSync ...
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/315#discussion_r129150747
--- Diff: src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java ---
@@ -1451,7 +1452,12 @@ public synchronized void restartLeaderElection(QuorumVerifier qvOLD, QuorumVerif
}
public String getNextDynamicConfigFilename() {
- return configFilename + QuorumPeerConfig.nextDynamicConfigFileSuffix;
+ if (configFilename != null) {
--- End diff --
Thanks for review, Alex. I'll look into making configFileName not null for this test, which is more deterministic than a random id generator.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---