ZOOKEEPER-3437: Improve sync throttling on a learner master by jhuan31 · Pull Request #995 · apache/zookeeper

jhuan31 · 2019-06-20T22:26:54Z

No description provided.

mayawang · 2019-06-25T21:40:27Z

Thank you for working on this!
Do we have design documentation on this issue other than the contents in JIRA? It will be really helpful to put some of the technical/design decisions in description for reviewers to learn more about the contexts.

I left a comment based on my understanding. :)

mayawang · 2019-06-25T21:39:48Z

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

+    private static final Logger LOG = LoggerFactory.getLogger(LearnerMaster.class);
+
+    // Throttle when there are too many concurrent snapshots being sent to observers
+    private static final String MAX_CONCURRENT_SNAPSYNCS = "zookeeper.leader.maxConcurrentSnapSyncs";


Would this configuration be shared between leader and observer master? If so, I think there are a few assumptions that worth a closer look.

Leader and Followers are running on machines with similar capability (CPU, network bandwidth). This assumption is usually true.

ObserverMasters and Observers are also running on machines with similar capabilities (CPU, network bandwidth). This assumption may not be true. E.g. If we deploy some Observers in a different data center than the ObserverMaster, then the effective network bandwidth between ObserverMaster and Observer could be significantly smaller than the bandwidth between Leader and Followers. In this case, we might want to set a different maxConcurrentSnapSyncs for ObserverMaster than Leader.

Do those assumptions make sense here?

The thing is, we don't know what the appropriate thresholds are until we roll the feature out and then tune the settings. If it turns out that we do need different settings for Leader and ObserverMaster, we can add it, instead of introducing a setting that might never be used. What do you think? (BTW, in our deployment, Leader and Followers can be in different data centers too.)

Thanks for the details! I think we can keep this configuration just one parameter for now. I'd love to learn the actual system behavior once this is used in production.

mayawang · 2019-06-25T21:40:01Z

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

+        return maxConcurrentSnapSyncs;
+    }
+
+    public void setMaxConcurrentSnapSyncs(int maxConcurrentSnapSyncs) {


I have not seen any usage of setMaxConcurrentSnapSyncs, and I am not sure how we can set different values for Leader vs ObserverMaster. How do we plan to use setMaxConcurrentSnapSyncs?

This function is called in FollwerBean and LeaderBean. No, we can't set different values for Leader and ObserverMaster. We did have a discussion whether to support different settings for Leader and ObserverMaster. Since we don't see a need for different settings and since we have too many flags/settings already, we decide to go with one setting for both Leader and ObserverMaster

jhuan31 · 2019-07-01T22:41:49Z

We don't have any design docs :( But you just remind me that I need to update the ZooKeeper Administrator's Guide :)

hanm · 2019-07-03T23:38:07Z

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

    public void shutdown() {
        // Send the packet of death
        try {
+            queuedPackets.clear();


This seems unrelated to this patch, but it looks the right thing to do. Just want to make sure this was not accidentally introduced.

Good catch! It is actually added in this patch to handle diff sync throttling. Diff txns are queued in syncFollower() before the throttle. Later when we decide that this diff sync should be throttled, the txns are in the queues already so we need to remove them.

hanm · 2019-07-03T23:44:33Z

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

+
+/**
+ * Utility class to limit the number of concurrent syncs from a leader to
+ * observers and followers.  {@link LearnerHandler} objects should call


nit: might add "or syncs from a follower to observers".

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

hanm · 2019-07-03T23:59:48Z

update the ZooKeeper Administrator's Guide

Yes it'll be great to include the update as part of this patch.

hanm

LGTM with some nits (and after doc is updated).

lvfangmin

Thanks @jhuan31, this looks good me.

I'll merge it back to master by end of this week if there is no objection.

eolivelli

Looks good to me as well.

but I left a very few minor comments, please fix them

eolivelli · 2019-07-23T04:59:54Z

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


            syncLimitCheck.start();
+            // sync ends when NEWLEADER-ACK is received
+            syncThrottler.endSync();


Is this endSync() call to be put in a finally block?

endSync() is called in the finally block line 720-722, if not called earlier.

eolivelli · 2019-07-23T05:11:55Z

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

+    }
+
+    @Test(expected = SyncThrottleException.class)
+    public void testTryWithResourceThrottle() throws Exception {


It is not very clear that we are testing that the second call to beginSync is the one that throws the exception
I suggest to use an explicit catch block and not use the 'expected' attribute

eolivelli · 2019-07-23T05:13:49Z

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

+
+        final LearnerSyncThrottler throttler =
+                new LearnerSyncThrottler(numThreads, syncType);
+        ExecutorService threadPool = Executors.newFixedThreadPool(numThreads);


Please shutdown this pool in a finally block

eolivelli

Looks good

eolivelli · 2019-07-24T16:02:21Z

retest maven build

Author: Jie Huang <jiehuang@fb.com> Reviewers: Michael Han <lhan@twitter.com>, Enrico Olivelli <eolivelli@apache.org>, Fangmin Lyu <fangmin@apache.org> Closes apache#995 from jhuan31/ZOOKEEPER-3437

mayawang reviewed Jun 25, 2019

View reviewed changes

hanm reviewed Jul 3, 2019

View reviewed changes

hanm approved these changes Jul 4, 2019

View reviewed changes

jhuan31 force-pushed the ZOOKEEPER-3437 branch from 01d71a8 to dbd817f Compare July 8, 2019 22:35

lvfangmin approved these changes Jul 22, 2019

View reviewed changes

eolivelli requested changes Jul 23, 2019

View reviewed changes

Jie Huang added 4 commits July 23, 2019 21:20

ZOOKEEPER-3437: Improve sync throttling on a learner master

ced9c27

spotbugs fix

84ab2e6

update doc and comments

74154c4

small fix the unit tests

d38b2fe

jhuan31 force-pushed the ZOOKEEPER-3437 branch from dbd817f to d38b2fe Compare July 24, 2019 05:39

eolivelli approved these changes Jul 24, 2019

View reviewed changes

asfgit closed this in f38905e Jul 25, 2019

Conversation

jhuan31 commented Jun 20, 2019

Uh oh!

mayawang commented Jun 25, 2019

Uh oh!

mayawang Jun 25, 2019 • 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!

Choose a reason for hiding this comment

Uh oh!

jhuan31 commented Jul 1, 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!

hanm commented Jul 3, 2019

Uh oh!

hanm left a comment

Choose a reason for hiding this comment

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eolivelli left a comment

Choose a reason for hiding this comment

Uh oh!

eolivelli commented Jul 24, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mayawang Jun 25, 2019 •

edited

Loading