ZOOKEEPER-3574: Close quorum socket asynchronously to avoid server sh… #1115

jhuan31 · 2019-10-13T05:20:40Z

…utdown stalled by long socket closing time

jhuan31 · 2019-11-12T23:19:50Z

retest maven build

zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/Learner.java

jhuan31 · 2019-11-15T19:19:35Z

retest maven build

asf-ci · 2019-11-25T17:43:15Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build-maven/1658/

asf-ci · 2019-12-11T21:02:03Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build-maven/1714/

Failed Tests: 1

PreCommit-ZOOKEEPER-github-pr-build-maven/org.apache.zookeeper:zookeeper: 1

org.apache.zookeeper.test.DisconnectedWatcherTest.testManyChildWatchersAutoReset

jhuan31 · 2019-12-12T04:07:57Z

retest maven build

asf-ci · 2019-12-12T05:15:35Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build-maven/1716/

asf-ci · 2019-12-17T17:48:09Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build-maven/1748/

lvfangmin · 2019-12-20T17:36:33Z

Manually restarted the job on Jenkins and it passed.

zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/Learner.java

anmolnar

Few nitpicks

anmolnar · 2020-01-29T13:37:26Z

zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/Learner.java


    protected Socket sock;
    protected MultipleAddresses leaderAddr;
+    protected AtomicBoolean sockBeingClosed = new AtomicBoolean(false);


I think socketClosed would be a better name for this variable, because it's not only guarding the "closing" method.

It is ONLY guarding the closing method. When it is set, the socket might not be closed for a long time, e.g. 30 seconds. But I don't want to name it "socketClosed" because it doesn't mean the socket is closed. Thoughts?

anmolnar · 2020-01-29T13:38:49Z

zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/Learner.java

+    void closeSockSync() {
+        try {
+            long startTime = Time.currentElapsedTime();
+            sock.close();


You might want to add 2 more things here:

double check if sock is still not null

make sock = null after the close to prevent further usage of the object and let GC collect it

belugabehr · 2020-01-30T23:48:00Z

Can you please introduce a CachedThreadPool so that a new thread does not need to be spawned for every socket?

https://docs.oracle.com/javase/7/docs/api/java/util/concurrent/Executors.html#newCachedThreadPool()

belugabehr · 2020-01-30T23:52:27Z

Also, better than using a conditional statement in the close method:

            if (sockBeingClosed.compareAndSet(false, true)) {
                if (closeSocketAsync) {
                    final Thread closingThread = new Thread(() -> closeSockSync(), "CloseSocketThread(sid:" + zk.getServerId());
                    closingThread.setDaemon(true);
                    closingThread.start();
                } else {
                    closeSockSync();
                }

Always use a ThreadPool, but use a single-thread thread pool or a 'direct' thread pool:

https://guava.dev/releases/19.0/api/docs/com/google/common/util/concurrent/MoreExecutors.html#directExecutor()

jhuan31 · 2020-02-26T22:18:01Z

Can you please introduce a CachedThreadPool so that a new thread does not need to be spawned for every socket?

https://docs.oracle.com/javase/7/docs/api/java/util/concurrent/Executors.html#newCachedThreadPool()

This is for closing the quorum socket once in a while, not the client sockets. I check the link you provide and it says the thread will be terminated if not being used for 60 seconds--I would expect the life time of a learner much longer than that. So I don't see any benefit of using a thread pool, or do I miss anything?

jhuan31 · 2020-02-26T22:24:03Z

Also, better than using a conditional statement in the close method:
            if (sockBeingClosed.compareAndSet(false, true)) {
                if (closeSocketAsync) {
                    final Thread closingThread = new Thread(() -> closeSockSync(), "CloseSocketThread(sid:" + zk.getServerId());
                    closingThread.setDaemon(true);
                    closingThread.start();
                } else {
                    closeSockSync();
                }
Always use a ThreadPool, but use a single-thread thread pool or a 'direct' thread pool:

https://guava.dev/releases/19.0/api/docs/com/google/common/util/concurrent/MoreExecutors.html#directExecutor()

The conditional statement is to preserve the current "blocking" behavior in case it's needed. How do I do that using a thread pool?

lvfangmin

+1

Thanks @jhuan31.

eolivelli

I will merge soon
Thank you guys!

ztzg

LGTM, but 1. untested, and 2. see the documentation question.

ztzg · 2020-03-30T10:38:51Z

zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/Learner.java

+    public static final String LEARNER_CLOSE_SOCKET_ASYNC = "learner.closeSocketAsync";
+    public static final boolean closeSocketAsync = Boolean.getBoolean(LEARNER_CLOSE_SOCKET_ASYNC);


Not trying to block this PR, and things can be improved later, but:

I see that such properties, including learner.asyncSending above it, are not documented. Is there a specific policy regarding which properties are to be mentioned in zookeeperAdmin.md? I am wondering how one can learn about all these knobs, and their raison d'être? (I know the corresponding ticket ID can be found via Git, but that incurs some overhead.)

Thanks for your comments. I will add the flag to zookeeperAdmin. As for test, I find it is hard to construct a unit test for this. But this feature has been enabled in our production for months.

@jhuan31: By "untested," I meant: I have looked into the code, but I haven't tested it :) I understand that it is not always easy to create meaningful tests for socket behavior. Thank you for taking care of the documentation.

…utdown stalled by long socket closing time

eolivelli

@ztzg as rule of thumb we have to document each know.
Usually once we introduce a know we are no more going to drop it.

jhuan31 · 2020-05-03T15:52:24Z

I will merge soon
Thank you guys!

@eolivelli Does this PR need more changes? Is it ready for merge? I refer "learner.closeSockeAsync" in a later PR #1301 and @symat points out that he can't find it in the code (since this PR is not merged yet) :)

eolivelli

LGTM

Sorry for the delay.
I can merge as soon as I back to work tomorrow

…utdown stalled by long socket closing time …utdown stalled by long socket closing time Author: Jie Huang <[email protected]> Reviewers: Enrico Olivelli <[email protected]>, Fangmin Lyu <[email protected]> Closes apache#1115 from jhuan31/ZOOKEEPER-3574

jhuan31 force-pushed the ZOOKEEPER-3574 branch from 2c8dc40 to 44fc0e7 Compare October 26, 2019 16:11

eolivelli requested changes Nov 13, 2019

View reviewed changes

lvfangmin mentioned this pull request Nov 14, 2019

[ZOOKEEPER-3384] Add SO_LINGER socket option to avoid long quorum unavailable during close with full send buffer #1085

Closed

jhuan31 force-pushed the ZOOKEEPER-3574 branch from cb8ac1b to 939a486 Compare November 25, 2019 17:02

jhuan31 force-pushed the ZOOKEEPER-3574 branch from 939a486 to 0473b08 Compare December 11, 2019 20:02

lvfangmin reviewed Dec 20, 2019

View reviewed changes

zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/Learner.java Outdated Show resolved Hide resolved

jhuan31 force-pushed the ZOOKEEPER-3574 branch from eb6add0 to 5d233e2 Compare January 6, 2020 22:43

anmolnar reviewed Jan 29, 2020

View reviewed changes

jhuan31 force-pushed the ZOOKEEPER-3574 branch 2 times, most recently from ed3a9d5 to c70965a Compare February 26, 2020 22:30

lvfangmin approved these changes Mar 26, 2020

View reviewed changes

eolivelli approved these changes Mar 28, 2020

View reviewed changes

ztzg approved these changes Mar 30, 2020

View reviewed changes

Jie Huang added 2 commits April 4, 2020 15:33

ZOOKEEPER-3574: Close quorum socket asynchronously to avoid server sh…

ee9ae54

…utdown stalled by long socket closing time

update the doc

1025489

jhuan31 force-pushed the ZOOKEEPER-3574 branch from c38a69a to 1025489 Compare April 4, 2020 22:35

eolivelli reviewed Apr 5, 2020

View reviewed changes

jhuan31 mentioned this pull request May 3, 2020

ZOOKEEPER-3774: Close quorum socket asynchronously on the leader to a… #1301

Closed

eolivelli approved these changes May 3, 2020

View reviewed changes

eolivelli closed this in b4f6e82 May 3, 2020

		public static final String LEARNER_CLOSE_SOCKET_ASYNC = "learner.closeSocketAsync";
		public static final boolean closeSocketAsync = Boolean.getBoolean(LEARNER_CLOSE_SOCKET_ASYNC);

ZOOKEEPER-3574: Close quorum socket asynchronously to avoid server sh… #1115

ZOOKEEPER-3574: Close quorum socket asynchronously to avoid server sh… #1115

Uh oh!

Conversation

jhuan31 commented Oct 13, 2019

Uh oh!

jhuan31 commented Nov 12, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jhuan31 commented Nov 15, 2019

Uh oh!

asf-ci commented Nov 25, 2019

Uh oh!

asf-ci commented Dec 11, 2019

Failed Tests: 1

PreCommit-ZOOKEEPER-github-pr-build-maven/org.apache.zookeeper:zookeeper: 1

Uh oh!

jhuan31 commented Dec 12, 2019

Uh oh!

asf-ci commented Dec 12, 2019

Uh oh!

asf-ci commented Dec 17, 2019

Uh oh!

lvfangmin commented Dec 20, 2019

Uh oh!

Uh oh!

anmolnar left a comment

Choose a reason for hiding this comment

Uh oh!

anmolnar Jan 29, 2020

Choose a reason for hiding this comment

Uh oh!

jhuan31 Feb 26, 2020

Choose a reason for hiding this comment

Uh oh!

anmolnar Jan 29, 2020

Choose a reason for hiding this comment

Uh oh!

jhuan31 Feb 26, 2020

Choose a reason for hiding this comment

Uh oh!

belugabehr commented Jan 30, 2020

Uh oh!

belugabehr commented Jan 30, 2020

Uh oh!

jhuan31 commented Feb 26, 2020

Uh oh!

jhuan31 commented Feb 26, 2020

Uh oh!

lvfangmin left a comment

Choose a reason for hiding this comment

Uh oh!

eolivelli left a comment

Choose a reason for hiding this comment

Uh oh!

ztzg left a comment

Choose a reason for hiding this comment

Uh oh!

ztzg Mar 30, 2020

Choose a reason for hiding this comment

Uh oh!

jhuan31 Apr 4, 2020

Choose a reason for hiding this comment

Uh oh!

ztzg Apr 6, 2020

Choose a reason for hiding this comment

Uh oh!

eolivelli left a comment

Choose a reason for hiding this comment

Uh oh!

jhuan31 commented May 3, 2020

Uh oh!

eolivelli left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels