Fix huge number of watches in zk by asdf2014 · Pull Request #9172 · apache/druid

asdf2014 · 2020-01-13T11:07:40Z

Description

Fix huge number of watches in zk
Tear down nodeAnnouncer
Remove useless Logger and ExecutorService
Init CuratorListener by lambda
Improve explicit type
Using CuratorMultiTransaction instead of CuratorTransaction
Add @GuardedBy("toAnnounce") for toUpdate field
Improve docs

Related to #6683

This PR has:

been self-reviewed.
- using the concurrency checklist (Remove this item if the PR doesn't have any relation to concurrency.)
added documentation for new or modified features or behaviors.
added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
added or updated version, license, or notice information in licenses.yaml
added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
added unit tests or modified existing tests to cover new code paths.
added integration tests.
been tested in a test Druid cluster.

@GuardedBy

* Tear down nodeAnnouncer * Remove useless Logger and ExecutorService * Init CuratorListener by lambda * Improve explicit type * Using CuratorMultiTransaction instead of CuratorTransaction * Add @GuardedBy("toAnnounce") for toUpdate field * Improve docs

leventov

Please also apply the same improvements to Announcer when possible, as the ones suggested for NodeAnnouncer here, as well as these improvements already applied by @kaijianding and you to this moment, in the course of #6683.

leventov · 2020-01-13T12:47:48Z

+import java.util.concurrent.CopyOnWriteArrayList;
+
+/**
+ * NodeAnnouncer announces single node on Zookeeper and only watches this node,


Please add a mirroring comment in Announcer

leventov · 2020-01-13T13:11:41Z

+  private final ConcurrentMap<String, NodeCache> listeners = new ConcurrentHashMap<>();
+  private final ConcurrentMap<String, byte[]> announcedPaths = new ConcurrentHashMap<>();
+  /**
+   * Only the one created the parent path can drop the parent path, so should remember these created parents.


This comment sounds confusing, shouldn't one of "parent path" occurrences in it be something else?

leventov · 2020-01-13T13:15:33Z

+
+      started = false;
+
+      Closer closer = Closer.create();


I think you can add unannouncements, and pathsCreatedInThisAnnouncer deletion operation to the Closer too, so they all attempted in case of problems.

Sorry, I don't understand this comment. These delete operations are not closable, can they be registered in Closer?

Like closer.register(() -> unannounce(path))

Thanks, I will patch this comment 👍

I guess this change hasn't been applied yet?

leventov · 2020-01-13T13:15:53Z

+        unannounce(announcementPath);
+      }
+
+      if (!pathsCreatedInThisAnnouncer.isEmpty()) {


Please extract this as a method

Is it really necessary? 😰

It's optional

Okay, let's stay the original way 😄

leventov · 2020-01-13T13:20:37Z

+        }
+      }
+      catch (Exception e) {
+        log.debug(e, "Problem checking if the parent existed, ignoring.");


Please change the comment ", assuming it doesn't exist."

leventov · 2020-01-13T13:38:36Z

+
+    final byte[] billy = StringUtils.toUtf8("billy");
+    final String testPath = "/somewhere/test2";
+    final String parent = ZKPaths.getPathAndNode(testPath).getPath();


leventov · 2020-01-13T13:41:28Z

+ * NodeAnnouncer announces single node on Zookeeper and only watches this node,
+ * while {@link Announcer} watches all child paths, not only this node
+ */
+public class NodeAnnouncer


Please add a concurrent control flow documentation explaining how and why somebody may call announce() and update() concurrently or before start().

Okay, I will improve this doc later.

leventov · 2020-01-13T13:43:02Z

+    }
+  }
+
+  private void createPath(String parentPath, boolean removeParentsIfCreated)


Please annotate @GuardedBy("toAnnounce")

leventov · 2020-01-13T13:43:52Z

+  /**
+   * Only the one created the parent path can drop the parent path, so should remember these created parents.
+   */
+  private final List<String> pathsCreatedInThisAnnouncer = new CopyOnWriteArrayList<>();


Please annotate @GuardedBy("toAnnounce")

Doesn't need to be CopyOnWriteArrayList, can be a simple ArrayList.

leventov · 2020-01-13T13:45:40Z

+    try {
+      curator.create().creatingParentsIfNeeded().forPath(parentPath);
+      if (removeParentsIfCreated) {
+        pathsCreatedInThisAnnouncer.add(parentPath);


The path is added pathsCreatedInThisAnnouncer regardless of whether it was actually created two lines above or already existed?

I think so, what's your concern?

Because then the path is not actually paths**Created**InThisAnnouncer

This should be resolved before merge

pzhdfy · 2020-01-14T04:35:15Z

this is similar to #6683?

asdf2014 · 2020-01-14T07:32:28Z

@pzhdfy Yep, the original discussion is here: https://lists.apache.org/thread.html/r92fcfa896418b941dd4aa1eed7b60aaf5b7e2ea55137600d844ff4a4%40%3Cdev.druid.apache.org%3E

pzhdfy · 2020-01-14T10:09:17Z

@pzhdfy Yep, the original discussion is here: https://lists.apache.org/thread.html/r92fcfa896418b941dd4aa1eed7b60aaf5b7e2ea55137600d844ff4a4%40%3Cdev.druid.apache.org%3E

We use #6683 in our production environment, and it works great fine. The watch count decrease more than 90% .

So I raise this pr to be merged.

jihoonson

Hi @asdf2014, thank you for taking up this issue! I left a couple of comments. Besides them, I would like to say, it could be better if we share some common codes between NodeAnnouncer and Announcer. But I would regard this comment as a nit and don't mind if the refactoring would have done in a followup PR.

jihoonson · 2020-01-21T23:51:28Z

+import java.util.concurrent.ConcurrentMap;
+
+/**
+ * {@link NodeAnnouncer} announces single node on Zookeeper and only watches this node,


typo: announces a single node.

jihoonson · 2020-01-21T23:57:37Z

@@ -64,6 +67,7 @@ public class Announcer
  private final ExecutorService pathChildrenCacheExecutor;

  private final List<Announceable> toAnnounce = new ArrayList<>();


Please add @GuaredBy("toAnnounce") for this list too.

jihoonson · 2020-01-21T23:57:45Z

+   * In case a path is added to this collection in {@link #announce} before zk is connected,
+   * should remember the path and do announce in {@link #start} later.
+   */
+  private final List<Announceable> toAnnounce = new ArrayList<>();


Please add @GuaredBy("toAnnounce") for this list too.

jihoonson · 2020-01-22T00:02:48Z

+  private final ConcurrentMap<String, NodeCache> listeners = new ConcurrentHashMap<>();
+  private final ConcurrentMap<String, byte[]> announcedPaths = new ConcurrentHashMap<>();
+  /**
+   * Only the one created the parent path can drop it, so should remember these created parents.


How about rephrasing such as This list is to remember all paths this node announcer has created. On {@list #stop}, the node announcer is responsible for deleting all paths in this list.

Thanks. It sounds better. Also, I use {@link #stop} instead of {@list #stop}.

jihoonson · 2020-01-22T00:04:34Z

+
+      started = false;
+
+      Closer closer = Closer.create();


I guess this change hasn't been applied yet?

jihoonson · 2020-01-22T00:20:42Z

+      try {
+        if (!Arrays.equals(oldBytes, bytes)) {
+          announcedPaths.put(path, bytes);
+          updateAnnouncement(path, bytes);


I guess it would be probably worth to define the concurrency control more precisely across all variables and methods in this class. I'm not sure why listeners is a concurrentHashMap since it's guarded by toAnnounce. Similar question for announcedPaths. Seems like it's a concurrentHashMap to allow reads without locking toAnnounce. However, unannounce() updates announcedPaths without the lock on toAnnounce. Maybe they don't have to be concurrentHashMaps. Or maybe there is another way to provide a better concurrency control.

asdf2014 · 2020-01-23T13:34:52Z

Hi, @jihoonson. Thank you and @leventov for your comments. They are very helpful. I would prefer to raise another one for a larger refactoring, so we can resolve this issue ASAP.

jihoonson · 2020-01-23T21:08:06Z

Hi, @jihoonson. Thank you and @leventov for your comments. They are very helpful. I would prefer to raise another one for a larger refactoring, so we can resolve this issue ASAP.

Ok. Please consider the latest comments from me and @leventov.

asdf2014 · 2020-01-27T07:54:30Z

@jihoonson Okay, no problem. Thanks also to @leventov for creating the #9244 issue.

jihoonson · 2020-01-28T19:59:19Z

@asdf2014 thanks. Let me know when this PR is ready for another review.

jihoonson · 2020-03-20T22:55:47Z

Hi @asdf2014, have you had a chance to address the comments from me and @leventov?

asdf2014 · 2020-03-23T02:58:40Z

@jihoonson Sure, I will try to continue to address these comments. Thanks for reminding.

jihoonson · 2020-03-23T17:00:23Z

@asdf2014 thank you!

asdf2014 · 2020-03-24T03:26:07Z

@jihoonson You are welcome!

stale · 2020-05-30T06:12:32Z

This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 4 weeks if no further activity occurs. If you think that's incorrect or this pull request should instead be reviewed, please simply write any comment. Even if closed, you can still revive the PR at any time or discuss it on the dev@druid.apache.org list. Thank you for your contributions.

stale · 2020-06-27T07:45:39Z

This pull request/issue has been closed due to lack of activity. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.

stale · 2020-06-27T18:44:51Z

This pull request/issue is no longer marked as stale.

stale · 2020-08-29T08:54:00Z

This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 4 weeks if no further activity occurs. If you think that's incorrect or this pull request should instead be reviewed, please simply write any comment. Even if closed, you can still revive the PR at any time or discuss it on the dev@druid.apache.org list. Thank you for your contributions.

stale · 2020-10-04T02:35:04Z

This pull request/issue has been closed due to lack of activity. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.

a2l007 · 2021-03-22T18:11:24Z

Hi @asdf2014 , are you planning to continue working on this PR?

asdf2014 added the Area - ZooKeeper/Curator label Jan 13, 2020

asdf2014 requested a review from leventov January 13, 2020 11:12

leventov requested changes Jan 13, 2020

View reviewed changes

asdf2014 added 2 commits January 20, 2020 16:08

Patch comments

8420d73

Add license

da8538b

jihoonson reviewed Jan 22, 2020

View reviewed changes

Patch comments

67b5064

leventov mentioned this pull request Jan 23, 2020

Add concurrent access and control flow documentation to Announcer and NodeAnnouncer #9244

Open

Merge branch 'master' into tmp

23cd4d8

stale Bot added the stale label May 30, 2020

stale Bot closed this Jun 27, 2020

jihoonson reopened this Jun 27, 2020

stale Bot removed the stale label Jun 27, 2020

stale Bot added the stale label Aug 29, 2020

stale Bot closed this Oct 4, 2020

GWphua mentioned this pull request Nov 15, 2024

Fix Huge Number of Watches in ZooKeeper #17482

Merged

8 tasks

		@@ -64,6 +67,7 @@ public class Announcer
		private final ExecutorService pathChildrenCacheExecutor;

		private final List<Announceable> toAnnounce = new ArrayList<>();

Conversation

asdf2014 commented Jan 13, 2020

Description

Uh oh!

leventov 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!

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!

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!

leventov Jan 23, 2020 • 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!

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!

pzhdfy commented Jan 14, 2020

Uh oh!

asdf2014 commented Jan 14, 2020

Uh oh!

pzhdfy commented Jan 14, 2020

Uh oh!

jihoonson 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!

leventov Jan 23, 2020 •

edited

Loading