Skip to content
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

Jump to bottom

ZOOKEEPER-3042: testFailedTxnAsPartOfQuorumLoss is flaky #521

Closed
wants to merge 2 commits into from
Closed

ZOOKEEPER-3042: testFailedTxnAsPartOfQuorumLoss is flaky #521

wants to merge 2 commits into from

Conversation

lavacat
Copy link
Contributor

@lavacat lavacat commented May 14, 2018

  • relaxed check of outstanding proposals queue
  • close clients after restart
  • restart client after old leader restart

@lavacat lavacat force-pushed the testFailedTxnAsPartOfQuorumLoss-fix branch 6 times, most recently from 19c44eb to c8646e6 Compare May 15, 2018 17:34
anmolnar
anmolnar approved these changes May 16, 2018
View reviewed changes
Copy link
Contributor

@anmolnar anmolnar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 lgtm, just a small suggestion

src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerMainTest.java Outdated
// there can be extra sessionClose proposals
Assert.assertTrue(outstanding.size() > 0);
Proposal p = findProposalOfType(outstanding, OpCode.create);
Assert.assertNotNull(p);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add some message to the assert here to explain what could have gone wrong.

@lavacat lavacat force-pushed the testFailedTxnAsPartOfQuorumLoss-fix branch from c8646e6 to 44dd195 Compare May 17, 2018 04:31
@lavacat
ZOOKEEPER-3042: testFailedTxnAsPartOfQuorumLoss is flaky
427ab8c 
- relaxed check of outstanding proposals queue
- close clients after restart
- restart client after old leader restart
@lavacat lavacat force-pushed the testFailedTxnAsPartOfQuorumLoss-fix branch 2 times, most recently from 9a5e1df to d1f9d44 Compare May 17, 2018 08:14
@lavacat
ZOOKEEPER-1932: ignore LETest
c361efa 
Motivation:
- LETest is flaky
- LeaderElection is already deprecated and removed in trunk
@lavacat lavacat force-pushed the testFailedTxnAsPartOfQuorumLoss-fix branch from d1f9d44 to c361efa Compare May 17, 2018 08:39
@lavacat
Copy link
Contributor Author

lavacat commented May 17, 2018

@anmolnar thanks, updated

@lavacat
Copy link
Contributor Author

lavacat commented May 31, 2018

@anmolnar Are there any concerns about this PR? I can remove "ignore LETest" commit. It was failing for me multiple times on PR test runs, but it's not on Flaky Dashboard.

testFailedTxnAsPartOfQuorumLoss is still listed as flaky on Dashboard, and I think this will fix it

@anmolnar
Copy link
Contributor

anmolnar commented Jun 1, 2018

@lavacat I think you're good to go as it is now. I've just become a committer recently, once I learn how to use the commit script, I'm happy to merge this. ;)

@maoling
Copy link
Member

maoling commented Jul 9, 2018

ping @anmolnar

@anmolnar
Copy link
Contributor

anmolnar commented Jul 9, 2018

@maoling Thanks for the heads-up.
@lavacat What are the target branches of this patch? Is this 3.5-only?

@lavacat
Copy link
Contributor Author

lavacat commented Jul 9, 2018

@anmolnar yes, only 3.5
There are some trivial conflicts on 3.4 and master. Change is still valid. I can create PRs. Do you want them now or after this is merged?

asfgit pushed a commit that referenced this pull request Jul 10, 2018
@lavacat @anmolnar
ZOOKEEPER-3042: testFailedTxnAsPartOfQuorumLoss is flaky
f1429d0 
- relaxed check of outstanding proposals queue
- close clients after restart
- restart client after old leader restart

Author: Bogdan Kanivets <bkanivets@gmail.com>

Reviewers: Andor Molnar <andor@apache.org>

Closes #521 from lavacat/testFailedTxnAsPartOfQuorumLoss-fix and squashes the following commits:

c361efa [Bogdan Kanivets] ZOOKEEPER-1932: ignore LETest
427ab8c [Bogdan Kanivets] ZOOKEEPER-3042: testFailedTxnAsPartOfQuorumLoss is flaky
@anmolnar
Copy link
Contributor

anmolnar commented Jul 10, 2018 •
edited

Committed to 3.5 branch. Thanks @lavacat
Please create separate pull request for the branches you'd like to merge this too.
And please close this PR.

@lavacat lavacat closed this Jul 11, 2018
@maoling
Copy link
Member

maoling commented Jul 15, 2018

@lavacat Jenkins still comlains about the flaky test in branch-3.4 and master.Could you plz fix them?

@anmolnar
Copy link
Contributor

@lavacat I second that. It would be great to fix this on all branches.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anmolnar anmolnar anmolnar approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@lavacat @anmolnar @maoling