ZOOKEEPER-3575: Moving sending packets in Learner to a separate thread #1116

jhuan31 · 2019-10-13T05:43:25Z

No description provided.

jhuan31 · 2019-10-14T16:22:47Z

retest maven build

jhuan31 · 2019-10-14T22:27:14Z

retest maven build

maoling · 2019-10-15T09:13:20Z

@eolivelli
Jenkins had broken down?

eolivelli · 2019-10-15T09:31:10Z

Yes.
We should fix the config.
I will do it as soon as possible, we have to switch to maven 3.6.2

lvfangmin · 2019-11-12T23:40:52Z

retest maven build

lvfangmin · 2019-11-13T00:42:46Z

@eolivelli we didn't see the 'restart' button on the Jenkins page, any permission we need to set up?

hanm · 2019-11-18T01:45:41Z

we didn't see the 'restart' button on the Jenkins page, any permission we need to set up?

@lvfangmin You might need administrative permission to manage Jenkins (create new project, configure, and restart jobs...). See https://cwiki.apache.org/confluence/display/INFRA/Jenkins for more details.

eolivelli · 2019-11-18T07:13:26Z

@lvfangmin @fpj can grant you acess (only the PMCC as far as I know)

eolivelli · 2019-12-04T09:26:37Z

zookeeper-server/src/test/java/org/apache/zookeeper/server/quorum/LearnerMetricsTest.java


    @Test
    public void testLearnerMetricsTest() throws Exception {
+        Learner.setAsyncSending(asyncSending);


please reset the value in a @before method
it is a static flag, so it will affect other tests

I assume you mean "reset in @after?"

eolivelli · 2019-12-04T09:26:49Z

...er-server/src/test/java/org/apache/zookeeper/server/quorum/ReconfigDuringLeaderSyncTest.java

    @Before
    public void setup() {
        System.setProperty("zookeeper.DigestAuthenticationProvider.superDigest", "super:D/InIHSb7yEEbrWz8b9l71RjZJU="/* password is 'test'*/);
+        Learner.setAsyncSending(asyncSending);


please reset the value in a @before method
it is a static flag, so it will affect other tests

reset in @afterclass

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

eolivelli · 2019-12-04T09:29:52Z

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

+                    break;
+                }
+
+                learner.messageTracker.trackSent(p.getType());


here https://github.com/apache/zookeeper/pull/1116/files#diff-c8b414c1ca2084ecb9fe32a0a3832d44R170
we accessing learner internal variables inside a synchronized block.
it looks like an inconsistent synchronization

maybe it depends on the fact that this feature is enabled or not, but I am not sure we are 100% safe
can you please motivate or at least leave some comment for future inspection and understanding of the flow ?

Without LearnerSender, there might be more than one thread writing to leaderOs so synchronization is required.

With LearnerSender, there still might be more than one thread calling writePacket but they are writing to a BlockingQueue so it's fine. Reading from the queue and writing to leaderOs is one thread (the learner sender thread), so we don't need synchronization. It's like we synchronize on the blockingQueue implicitly.

This is tricky. I may have missed some thing. Let me know and I'll fix the code.

Can you add a minimal comment to explain this point?
I agree with your explanation

This looks good to me, it might be better to not visit the internal variable directly, but using things like get to make the code easier to manage and reason about though.

asf-ci · 2019-12-12T04:40:34Z

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

eolivelli

I left one last request to add a comment

@lvfangmin @hanm PTAL
We need one more reviewer

eolivelli · 2019-12-14T07:55:21Z

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

        self.closeAllConnections();
        self.adminServer.setZooKeeperServer(null);
+
+       if (sender != null) {


nit: diff shows a missing space

eolivelli · 2019-12-14T07:56:30Z

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

+                    break;
+                }
+
+                learner.messageTracker.trackSent(p.getType());


Can you add a minimal comment to explain this point?
I agree with your explanation

lvfangmin

+1

LGTM, thanks Jie for upstreaming this.

lvfangmin · 2019-12-20T17:27:27Z

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

+                    break;
+                }
+
+                learner.messageTracker.trackSent(p.getType());


This looks good to me, it might be better to not visit the internal variable directly, but using things like get to make the code easier to manage and reason about though.

eolivelli · 2019-12-20T18:59:19Z

Let's merge

jhuan31 · 2020-01-06T18:54:47Z

@eolivelli Added a comment to explain the synchronization on leaderOs.

lvfangmin

+1

LGTM, thanks @jhuan31, I'll merge this tomorrow if there is no objection.

lvfangmin · 2020-01-25T17:49:25Z

Merged to master, thanks @jhuan31!

Author: Jie Huang <[email protected]> Reviewers: [email protected], [email protected] Closes apache#1116 from jhuan31/ZOOKEEPER-3575

Author: Jie Huang <[email protected]> Reviewers: [email protected], [email protected] Closes apache#1116 from jhuan31/ZOOKEEPER-3575 (cherry picked from commit 7c1251d)

Author: Jie Huang <[email protected]> Reviewers: [email protected], [email protected] Closes apache#1116 from jhuan31/ZOOKEEPER-3575

ZOOKEEPER-3575: Moving sending packets in Learner to a separate thread

6eb5e81

jhuan31 force-pushed the ZOOKEEPER-3575 branch from 2f29cc7 to 6eb5e81 Compare October 26, 2019 15:32

Jie Huang added 3 commits November 13, 2019 08:18

fix style issues

6d380ba

fix spotbugs issue

ff19233

fix sender shutdown logic

859b83f

jhuan31 force-pushed the ZOOKEEPER-3575 branch from ddd33f8 to 859b83f Compare November 13, 2019 18:44

fix trailing space

2582ba8

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

eolivelli requested changes Dec 4, 2019

View reviewed changes

clean up aysncSending flag in unit tests

aeec3a4

eolivelli requested changes Dec 14, 2019

View reviewed changes

lvfangmin approved these changes Dec 20, 2019

View reviewed changes

nit fix and add comment

21e4f79

lvfangmin approved these changes Jan 23, 2020

View reviewed changes

asfgit closed this in 7c1251d Jan 25, 2020

ZOOKEEPER-3575: Moving sending packets in Learner to a separate thread #1116

ZOOKEEPER-3575: Moving sending packets in Learner to a separate thread #1116

Uh oh!

Conversation

jhuan31 commented Oct 13, 2019

Uh oh!

jhuan31 commented Oct 14, 2019

Uh oh!

jhuan31 commented Oct 14, 2019

Uh oh!

maoling commented Oct 15, 2019

Uh oh!

eolivelli commented Oct 15, 2019

Uh oh!

lvfangmin commented Nov 12, 2019

Uh oh!

lvfangmin commented Nov 13, 2019

Uh oh!

hanm commented Nov 18, 2019

Uh oh!

eolivelli commented Nov 18, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asf-ci commented Dec 12, 2019

Uh oh!

eolivelli left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lvfangmin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eolivelli commented Dec 20, 2019

Uh oh!

jhuan31 commented Jan 6, 2020

Uh oh!

lvfangmin left a comment

Choose a reason for hiding this comment

Uh oh!

lvfangmin commented Jan 25, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

eolivelli left a comment •

edited

Loading