fix huge number of watches in zk issue by kaijianding · Pull Request #6683 · apache/druid

kaijianding · 2018-11-29T18:33:06Z

fix #6647 .
I guess the origin Announcer.java is better performance for segment announcement, so I just keep it.

I guess the root cause of this issue is that each child path has a watch on zookeeper server even we only add listener at the parent path. When the path is like /druid/prod/announces/$host, the number of watches is n * n (each server watches every host including itself). Plus the path for lookup, the number is n * n * 2.

I test it in my product environment, the number of watches is reduced as expected.

leventov · 2018-12-05T17:23:18Z

log.error maybe

leventov · 2018-12-05T17:23:25Z

log.error maybe

If a production Druid cluster has issues I don't think cluster operators will be happy to see this humour in the logs.

leventov · 2018-12-05T17:25:50Z

Announcer should be a javadoc link

"which is different with Announcer" the idea is not complete. Please elaborate.

which is different with Announcer means NodeAnnouncer announces single node on Zookeeper and only watch this node, while Announcer watches all child path, not only this node

leventov · 2018-12-05T17:27:05Z

The name and the purpose of this field is not clear

Maybe it is clearer to call it pathsCreatedInThisAnnouncer? Is there anything specific about them being "parents"? They are parents of what?

This list is iterated only once. There is no win in using CopyOnWriteArrayList rather than a simple ArrayList protected by a lock.

leventov · 2018-12-05T17:27:28Z

How is this different from toAnnounce?

toUpdate is added when update() called before zk is connected, the path to update may or may not be added to toAnnounce before.
Most part of NodeAnnouncer's code logic is following the original Announcer.java

Please annotate @GuadedBy("toAnnounce").

leventov · 2018-12-05T17:48:56Z

What does "stop" mean here?

stop is the stop() method annotated with LifecycleStop

leventov · 2018-12-05T17:49:09Z

Same as above

the curator.xxx.forPath(parentPath) throws Exception in method signature, so we have to catch Exception

leventov · 2018-12-05T17:49:15Z

Same as above

leventov · 2018-12-05T17:49:34Z

Same as above

the NodeCache.start() throws Exception in method signature, so we have to catch Exception

leventov · 2018-12-05T17:49:59Z

Same as above

gianm · 2018-12-20T23:26:24Z

From #6647,

If there are 10000 realtime tasks(not single realtime job, but like 50 jobs), then each task will create 2 * 10000 watches, finally we will get 200M watches!!

This seems strange & unnecessary, do you know why the tasks are creating watches for each other? They shouldn't be watching each other, I think, since they don't have a good reason to. Only the coordinators and brokers should be watching tasks, since they're the only ones that care about them (for lookups & segment discovery purposes).

Does Announcer set up too many watches, for some reason?

kaijianding · 2018-12-24T16:58:06Z

@gianm I'm not familiar with curator code, but it seems because we watch the parent path to get child events, curator adds watch for each child path

jihoonson · 2019-01-02T22:49:42Z

Hi @kaijianding, I still don't understand why you have such many watchers. In Druid, only master nodes (brokers, coordinator, overlord if you use zk-based remoteTaskRunner) watch such announcements. For example, AbstractCuratorServerInventoryView watches /some/zk/path/announcements which is used only in brokers and the coordinator. Similarly, /some/zk/path/listeners/lookups/ is watched only by the coordinator (LookupCoordinatorManager). Are you seeing that other modules like tasks are also watching those zk paths?

kaijianding · 2019-01-04T12:01:36Z

@jihoonson Announcer.java is also used in realtime node to re-announce itself when temporary disconnecting to zk, Then a realtime node also watches /some/zk/path/announcements

leventov · 2019-01-10T11:47:35Z

@kaijianding most answers to my questions that you given should be transformed into code comments, or the code should be altered in some other way. The objective is to approach the state that if I read the code for the first time, none of my questions were arisen.

jihoonson · 2019-02-11T22:21:12Z

@kaijianding thanks. Would you please fix the conflicts and check @leventov's last comment?

kaijianding · 2019-02-13T16:58:21Z

@jihoonson @leventov @gianm conflicts are resolved and code comments are updated, do you have any more comment?

leventov · 2019-02-13T22:13:35Z

@kaijianding this comment: #6683 (comment) is mostly not addressed.

Please don't force push: https://github.com/apache/incubator-druid/blob/master/CONTRIBUTING.md#if-your-pull-request-shows-conflicts-with-master

kaijianding · 2019-02-14T02:21:57Z

@leventov I added comments to toAnnounce, toUpdate, parentsIBuilt, and modified some other comments to help better understanding. Could you tell me which part of comments you think can improve?

leventov · 2019-02-20T18:26:24Z

+
+  private final CuratorFramework curator;
+
+  // incase a path is added to `toAnnounce` in announce() before zk is connected,


Please use Javadoc /** ... */ comments for fields.

"In case", missing space.

Is this right that it's implied in this comment that announce() may be called concurrently by somebody? How is this possible before start()? Could you please add concurrent control flow documentation to announce() and start() methods?

leventov · 2019-02-20T18:27:10Z

+  // incase a path is added to `toAnnounce` in announce() before zk is connected,
+  // should remember the path and do announce in start() later
+  private final List<Announceable> toAnnounce = new ArrayList<>();
+  // incase a path is added to `toUpdate` in update() before zk is connected,


leventov · 2019-02-20T18:31:30Z

+
+  // incase a path is added to `toAnnounce` in announce() before zk is connected,
+  // should remember the path and do announce in start() later
+  private final List<Announceable> toAnnounce = new ArrayList<>();


Maybe it's clearer to call it toAnnounceInStart?

Please annotate @GuadedBy("toAnnounce"). Could use create a separate lock object in a field called "lock"?

leventov · 2019-02-20T18:39:27Z

+ * 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


NodeAnnouncer and Announcer share a lot of code. Is it possible to extract a common base class and either extend it as Announcer and NodeAnnouncer or just add a flag to that basic class's state and use something like BaseAnnouncer(watchAll=true) as Announcer and BaseAnnouncer(watchAll=true) as NodeAnnouncer?

Could you please add concurrent control flow documentation to this class? From what threads (thread pools, executors) each method may be called?

leventov · 2019-02-20T18:45:37Z

+  private final List<Announceable> toUpdate = new ArrayList<>();
+  private final ConcurrentMap<String, NodeCache> listeners = new ConcurrentHashMap<>();
+  private final ConcurrentMap<String, byte[]> announcedPaths = new ConcurrentHashMap<>();
+  // only who creates the parent path can drop the parent path, so should remmeber the created parents


"only who creates the parent path can drop the parent path" - "Who" here is a thread? A class? Is this rule imposed by the Curator framework?

leventov · 2019-02-20T20:29:34Z

Please annotate @GuadedBy("toAnnounce").

leventov · 2019-02-20T20:32:41Z

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


Looks like a race condition is possible between announcedPaths.get() and announcedPaths.put(), is that benign? If not, please rewrite the code using map.compute().

leventov · 2019-02-20T20:36:49Z

+
+    boolean created = false;
+    synchronized (toAnnounce) {
+      if (started) {


It seems that started flag is already checked at the beginning of this method, why it is checked again here?

leventov · 2019-02-20T20:37:13Z

+              }
+          );
+
+          if (started) {


It seems that started flag is already checked at the beginning of this method, why it is checked again here?

leventov · 2019-02-20T20:44:44Z

+      }
+      CloseQuietly.close(closer);
+
+      for (String announcementPath : announcedPaths.keySet()) {


Entries are removed via map.remove() while iterating over the map. I cannot find proof that this is safe; it would be better to remove them via iterator.remove().

leventov · 2019-03-15T21:16:37Z

@kaijianding do you plan to continue this work?

jihoonson · 2019-05-07T15:47:21Z

@kaijianding would you please fix the conflicts and check @leventov's latest comments? BTW, I'm untagging milestone since this issue is not necessarily a release blocker. Feel free to let me know if you think this should be.

stale · 2019-07-06T16:07:51Z

This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 1 week 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 · 2019-07-13T17:07:45Z

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.

pzhdfy · 2020-01-11T09:31:09Z

We use this patch in our production environment，the watch count significantly decreased .
And anything works fine, so I raise for this pr

stale · 2020-01-11T12:31:13Z

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

stale · 2020-03-14T04:33:23Z

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-04-11T04:39:40Z

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.

fjy added this to the 0.13.1 milestone Nov 30, 2018

leventov self-requested a review December 1, 2018 00:13

This was referenced Dec 1, 2018

coordinator lose watch on /druid/announcements due to unreasonable usage of PathChildrenCache #6597

Closed

Add SelfDiscoveryResource; rename org.apache.druid.discovery.NodeType to NodeRole #6702

Merged

gianm requested a review from jihoonson December 4, 2018 21:55

leventov requested changes Dec 5, 2018

View reviewed changes

jon-wei removed this from the 0.14.0 milestone Feb 12, 2019

fix huge number of watches in zk issue

0e9b7e7

kaijianding force-pushed the watch branch from b0c733c to 0e9b7e7 Compare February 13, 2019 16:55

leventov requested changes Feb 20, 2019

View reviewed changes

fjy added this to the 0.15.0 milestone Mar 11, 2019

jihoonson removed this from the 0.15.0 milestone May 7, 2019

stale Bot added the stale label Jul 6, 2019

stale Bot closed this Jul 13, 2019

leventov added the Area - ZooKeeper/Curator label Jan 11, 2020

leventov reopened this Jan 11, 2020

stale Bot removed the stale label Jan 11, 2020

asdf2014 mentioned this pull request Jan 13, 2020

Fix huge number of watches in zk #9172

Closed

9 tasks

stale Bot added the stale label Mar 14, 2020

stale Bot closed this Apr 11, 2020

GWphua mentioned this pull request Nov 15, 2024

Fix Huge Number of Watches in ZooKeeper #17482

Merged

8 tasks


		private final CuratorFramework curator;

		// incase a path is added to `toAnnounce` in announce() before zk is connected,

Conversation

kaijianding commented Nov 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

gianm commented Dec 20, 2018

Uh oh!

kaijianding commented Dec 24, 2018

Uh oh!

jihoonson commented Jan 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kaijianding commented Jan 4, 2019

Uh oh!

leventov commented Jan 10, 2019

Uh oh!

jihoonson commented Feb 11, 2019

Uh oh!

kaijianding commented Feb 13, 2019

Uh oh!

leventov commented Feb 13, 2019

Uh oh!

kaijianding commented Feb 14, 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!

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

kaijianding commented Nov 29, 2018 •

edited

Loading

jihoonson commented Jan 2, 2019 •

edited

Loading

pzhdfy commented Jan 11, 2020 •

edited

Loading