Skip to content

ZOOKEEPER-3240: Close socket on Learner shutdown to avoid dangling so…#996

Closed
enixon wants to merge 3 commits into
apache:masterfrom
enixon:learner-close-socket
Closed

ZOOKEEPER-3240: Close socket on Learner shutdown to avoid dangling so…#996
enixon wants to merge 3 commits into
apache:masterfrom
enixon:learner-close-socket

Conversation

@enixon

@enixon enixon commented Jun 21, 2019

Copy link
Copy Markdown

…cket

@enixon

enixon commented Jun 21, 2019

Copy link
Copy Markdown
Author

Recreating #767 . That pull request had to be reverted because it was causing QuorumPeerMainTest::testFailedTxnAsPartOfQuorumLoss to repeatedly fail. This change does not directly effect the logic of that test but it does seem to expose a flaw in how it is staged. I believe the new change addresses that.

eolivelli
eolivelli approved these changes Jun 22, 2019
View reviewed changes
Comment thread zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/Learner.java Outdated

void closeSocket() {
try {
if (sock != null && !sock.isClosed()) {

@eolivelli eolivelli Jun 22, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just curious: why checking 'isClosed' ? Can't we call 'close' multiple times?

@enixon enixon Jun 28, 2019

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

You're completely right, this check doesn't actually contribute anything - I'll remove it.

anmolnar
anmolnar reviewed Jun 22, 2019
View reviewed changes

@anmolnar anmolnar left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Question.

Comment thread zookeeper-server/src/test/java/org/apache/zookeeper/server/quorum/QuorumPeerMainTest.java
int previousTick = servers.mt[leader].main.quorumPeer.tickTime;
servers.mt[leader].main.quorumPeer.tickTime = LEADER_TIMEOUT_MS;
// let the previous tick on the leader exhaust itself so the new tick time takes effect
Thread.sleep(previousTick);

@anmolnar anmolnar Jun 22, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

How is this change related to this patch?

@enixon enixon Jun 28, 2019

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

It comes down to timing. This test needs the leader to lag behind the actions taking place on the other servers and this change gives the test the full advantage of the leader's changed tick time. It seems that closing the socket alters the timing such that the stale Leader gets an earlier awareness of its stale status without the change.

@nkalmar

nkalmar commented Jun 28, 2019

Copy link
Copy Markdown
Contributor

retest maven build

@enixon

enixon commented Jun 28, 2019

Copy link
Copy Markdown
Author

ReconfigFailureCasesTest::testLeaderTimesoutOnNewQuorum fails on this change because it relies on the old behavior to prop a Leader up long enough for reconfig to fail. I'm trying to find a way to reconstruct that same behavior - if I can't, I'll have to delete the test.

anmolnar
anmolnar approved these changes Jul 2, 2019
View reviewed changes

@anmolnar anmolnar left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

+1 lgtm
Please keep an eye on the test that you added sleep() to. I'm afraid we're facing a new flaky test.

@asfgit asfgit closed this in 96eefaf Jul 2, 2019
@anmolnar

anmolnar commented Jul 2, 2019

Copy link
Copy Markdown
Contributor

merged to master. thanks.

@enixon
enixon deleted the learner-close-socket branch July 2, 2019 17:45
stickyhipp pushed a commit to stickyhipp/zookeeper that referenced this pull request Aug 2, 2019
Brian Nixon @stickyhipp
ZOOKEEPER-3240: Close socket on Learner shutdown to avoid dangling so…
9a1f7c7 
…cket

Author: Brian Nixon <nixon@fb.com>

Reviewers: hanm@apache.org, eolivelli@apache.org, andor@apache.org

Closes apache#996 from enixon/learner-close-socket and squashes the following commits:

57f5e89 [Brian Nixon] delete ReconfigFailureCasesTest::testLeaderTimesoutOnNewQuorum as it relies on the fixed buggy behavior
1598b3a [Brian Nixon] remove extraneous check on socket status
381889d [Brian Nixon] ZOOKEEPER-3240: Close socket on Learner shutdown to avoid dangling socket
fanyang89 pushed a commit to fanyang89/zookeeper that referenced this pull request Mar 26, 2021
Brian Nixon @fanyang89
ZOOKEEPER-3240: Close socket on Learner shutdown to avoid dangling so…
917ae6c 
…cket

Author: Brian Nixon <nixon@fb.com>

Reviewers: hanm@apache.org, eolivelli@apache.org, andor@apache.org

Closes apache#996 from enixon/learner-close-socket and squashes the following commits:

57f5e89 [Brian Nixon] delete ReconfigFailureCasesTest::testLeaderTimesoutOnNewQuorum as it relies on the fixed buggy behavior
1598b3a [Brian Nixon] remove extraneous check on socket status
381889d [Brian Nixon] ZOOKEEPER-3240: Close socket on Learner shutdown to avoid dangling socket
fanyang89 pushed a commit to fanyang89/zookeeper that referenced this pull request Mar 28, 2021
Brian Nixon @fanyang89
ZOOKEEPER-3240: Close socket on Learner shutdown to avoid dangling so…
c2f2216 
…cket

Author: Brian Nixon <nixon@fb.com>

Reviewers: hanm@apache.org, eolivelli@apache.org, andor@apache.org

Closes apache#996 from enixon/learner-close-socket and squashes the following commits:

57f5e89 [Brian Nixon] delete ReconfigFailureCasesTest::testLeaderTimesoutOnNewQuorum as it relies on the fixed buggy behavior
1598b3a [Brian Nixon] remove extraneous check on socket status
381889d [Brian Nixon] ZOOKEEPER-3240: Close socket on Learner shutdown to avoid dangling socket
RokLenarcic pushed a commit to RokLenarcic/zookeeper that referenced this pull request Aug 31, 2022
Brian Nixon @RokLenarcic
ZOOKEEPER-3240: Close socket on Learner shutdown to avoid dangling so…
f51d221 
…cket

Author: Brian Nixon <nixon@fb.com>

Reviewers: hanm@apache.org, eolivelli@apache.org, andor@apache.org

Closes apache#996 from enixon/learner-close-socket and squashes the following commits:

57f5e89 [Brian Nixon] delete ReconfigFailureCasesTest::testLeaderTimesoutOnNewQuorum as it relies on the fixed buggy behavior
1598b3a [Brian Nixon] remove extraneous check on socket status
381889d [Brian Nixon] ZOOKEEPER-3240: Close socket on Learner shutdown to avoid dangling socket
RokLenarcic pushed a commit to RokLenarcic/zookeeper that referenced this pull request Sep 3, 2022
Brian Nixon @RokLenarcic
ZOOKEEPER-3240: Close socket on Learner shutdown to avoid dangling so…
2ee3d28 
…cket

Author: Brian Nixon <nixon@fb.com>

Reviewers: hanm@apache.org, eolivelli@apache.org, andor@apache.org

Closes apache#996 from enixon/learner-close-socket and squashes the following commits:

57f5e89 [Brian Nixon] delete ReconfigFailureCasesTest::testLeaderTimesoutOnNewQuorum as it relies on the fixed buggy behavior
1598b3a [Brian Nixon] remove extraneous check on socket status
381889d [Brian Nixon] ZOOKEEPER-3240: Close socket on Learner shutdown to avoid dangling socket
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hanm hanm hanm approved these changes
@anmolnar anmolnar anmolnar approved these changes
@eolivelli eolivelli eolivelli approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@enixon @nkalmar @anmolnar @hanm @eolivelli