Fix Huge Number of Watches in ZooKeeper #17482

GWphua · 2024-11-15T08:21:43Z

Description

This PR is built upon #6683 and #9172 and aims to reduce the number of ZooKeeper watch counts.

Fixed Huge Number of Watches in ZooKeeper

The current Announcer.java leverages on Apache Curator's PathChildrenCache. In its present form, the announcement mechanism watches the immediate parent of the specified path. This results in all child nodes under the parent path being monitored by the ZooKeeper ensemble, including sibling nodes and children of the specified path. This causes an unnecessarily large number of ZooKeeper watches to be produced.

The new NodeAnnouncer.java class is simply Announcer.java but leverages on NodeCache instead to watch a single node during announcement. By eliminating the watches on child nodes, this approach significantly reduces the total number of watch counts in ZooKeeper. Users can opt-in to use the new NodeAnnouncer by setting toggling the feature flag druid.zk.service.pathChildrenCacheStrategy=false.

Tests conducted on the production server also indicate a decrease in watch counts resulting from this change.

Note:
The use of the two different announcer classes simultaneously may result in a KeeperException.NotEmptyException. This happens when two nodes are sharing the same parent, and since both announcers do not have a full picture of the nodes it is watching, the exception will be thrown when the following occurs:

Announcer removes all of its tracked children nodes.
Thinking that after all the removal the parent node has no children anymore, Announcer tries to remove the parent node.
If NodeAnnouncer is still watching one or more child node, the attempt by Announcer in removing the parent node will result in the exception.

Documentation

Remove humor in error logs.
Add JavaDocs and comments within code to better describe the process.
Add documentations on how to enable NodeAnnouncer.

Refactoring

Shift Announceable class out of Announcer.
Rename Announcer.java to PathChildrenAnnouncer.java
Add Announcer interface to facilitate dependency injection for different flavours of caching strategies.
Refactor long methods by creating helper functions.
Add ZKPathsUtils.java to abstract the retrieval of ZooKeeper path and ZooKeeper node.

Release note

New: A new opt-in caching strategy is provided that uses a much smaller number of ZooKeeper watches for service announcement.

Key changed/added classes in this PR

Announcer.java -> PathChildrenAnnouncer.java
Announcer.java interface (new)
NodeAnnouncer.java
Announceable.java
AnnouncerModule.java
CuratorConfig.java
DirectExecutorAnnouncer & SingleThreadedAnnouncer annotations for Guice.
docs/configuration/index.md & .spelling for docs.
Related test files.

This PR has:

been self-reviewed.
- using the concurrency checklist (Remove this item if the PR doesn't have any relation to concurrency.)
a release note entry in the PR description.
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, ensuring the threshold for code coverage is met.
been tested in a test Druid cluster.

* 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

GWphua · 2024-12-11T08:59:39Z

Ready for review, PTAL @kaijianding @leventov @jihoonson @asdf2014 @gianm

GWphua · 2024-12-19T06:55:51Z

I did some benchmarking on a single cluster, deploying two druid instances into two namespaces. We can see a lowered memory usage in ZooKeeper after the change.

I conducted the benchmark by submitting 5 instances of the trips_xaa (3 files) datasources. Since the size of my cluster is not really large, we will not be seeing up to hundreds of millions of ZooKeeper watch counts, but at least this gives us an idea of what the count will be like for larger clusters:

Ingestion

Querying

I ran a python script to conduct 10 groups of queries on the ingested datasources. Each group of queries are repeated 500 times. To simulate a concurrent querying environment, I allocated 9 threads to send these queries.

cryptoe · 2025-02-10T10:49:50Z

This looks quite interesting. How has your experience been using this?
Can we feature flag this ?

GWphua · 2025-02-21T10:24:50Z

hi @cryptoe, I have been using this for a week, I have seen an overall of 6% decrease in memory usage for the entire cluster. In terms of CPU usage, there is an 7% increase.

This may be because I am no longer using a Druid cluster with lots of ZooKeeper activity anymore. Both streaming and batch tasks are ingesting properly after the change. I can work towards creating a feature flag for this so people with the resources can also try it out. 😄

kgyrtkirk · 2025-02-26T10:24:20Z

server/src/main/java/org/apache/druid/curator/announcement/Announcer.java

      // I don't have a watcher on this path yet, create a Map and start watching.
      announcements.putIfAbsent(parentPath, new ConcurrentHashMap<>());

-      // Guaranteed to be non-null, but might be a map put in there by another thread.
+      // Guaranteed to be non-null, but might be a map put in here by another thread.


note: this seems like the usecase of computeIfAbsent

refactored.

kgyrtkirk · 2025-02-26T10:27:31Z

server/src/main/java/org/apache/druid/curator/announcement/NodeAnnouncer.java

+  @GuardedBy("toAnnounce")
+  private NodeCache setupNodeCache(String path)
+  {
+    final NodeCache cache = new NodeCache(curator, path, true);


NodeCache seems to be deprecated ; could we start using a non-deprecated api - or is there a reason to use the deprecated one?

Replaced with the more recent CuratorCache implementation.

I have tried to update PathChildrenCache to CuratorCache too, but I have problem passing the integration tests when doing so. Since this PR focuses on NodeAnnouncer, I feel we can leave this for later.

server/src/test/java/org/apache/druid/curator/announcement/NodeAnnouncerTest.java

kgyrtkirk · 2025-02-26T10:35:53Z

server/src/test/java/org/apache/druid/curator/announcement/NodeAnnouncerTest.java

+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;


please don't use junit4 for newly added tests

Updated with JUnit5

kgyrtkirk · 2025-02-26T10:43:53Z

server/src/main/java/org/apache/druid/server/ZKPathsUtils.java

+  public static String getParentPath(String path)
+  {
+    return ZKPaths.getPathAndNode(path).getPath();
+  }
+
+  public static String getParentNode(String path)
+  {
+    return ZKPaths.getPathAndNode(path).getNode();
+  }


not sure about the value of these methods...they could be inlined - so that the ZK api is used directly

Removed ZKPathsUtil

kgyrtkirk · 2025-02-26T10:53:17Z

server/src/main/java/org/apache/druid/curator/announcement/NodeAnnouncer.java

+    final String parentPath = ZKPathsUtils.getParentPath(path);
+    byte[] announcedPayload = announcedPaths.get(path);
+
+    // If announcedPayload is null, this means that we have yet to announce this path.
+    // There is a possibility that the parent paths do not exist, so we check if we need to create the parent path first.
+    if (announcedPayload == null) {
+      boolean buildParentPath = false;
+      try {
+        buildParentPath = curator.checkExists().forPath(parentPath) == null;
+      }
+      catch (Exception e) {
+        log.warn(e, "Failed to check existence of parent path. Proceeding without creating parent path.");
+      }
+
+      // Synchronize to make sure that I only create a listener once.
+      synchronized (toAnnounce) {
+        if (!listeners.containsKey(path)) {
+          final NodeCache cache = setupNodeCache(path);
+
+          if (started) {
+            if (buildParentPath) {
+              createPath(parentPath, removeParentIfCreated);
+            }
+            startCache(cache);
+            listeners.put(path, cache);
+          }
+        }
+      }
+    }


can you pack this into a method like ensureParentPathExists or something?

I find the Exception handling pretty strange here and in the createPath method
don't really understand why continue if curator.checkExists().forPath(parentPath) returns some exception ?

I referred to #195 when writing this. Seems like the exception does not affect the functionality, so I retained the behaviour from Announcer.

kgyrtkirk · 2025-02-26T10:57:26Z

server/src/main/java/org/apache/druid/curator/announcement/NodeAnnouncer.java

+  {
+    synchronized (toAnnounce) {
+      if (!started) {
+        return false; // Do nothing if not started


could the following lead to lost events:

L212 started is true

stop() is called -> sets started to false

L293 return false

is this a valid usecase? can an announcer started/stopped multiple times?
(I think not) ; so it might have been better to just throw an error here; or remove it entirely

Made necessary changes.

kgyrtkirk · 2025-02-26T10:58:34Z

server/src/main/java/org/apache/druid/curator/announcement/NodeAnnouncer.java

+              createPath(parentPath, removeParentIfCreated);
+            }
+            startCache(cache);
+            listeners.put(path, cache);


I don't think there is an upper limit on the number of cache-s ; could that cause any problems?

I don't think we will reach (2^32) - 1 elements. Besides, this PR aims to reduce memory consumption, so we should be safe.

kgyrtkirk · 2025-02-26T11:05:46Z

server/src/main/java/org/apache/druid/curator/announcement/NodeAnnouncer.java

+    final byte[] updatedAnnouncementData = announcedPaths.compute(path, (key, oldBytes) -> {
+      if (oldBytes == null) {
+        return bytes; // Insert the new value
+      } else if (!Arrays.equals(oldBytes, bytes)) {
+        throw new IAE("Cannot reannounce different values under the same path.");
+      }
+      return oldBytes; // No change if values are equal
+    });
+
+    // Return true if we have updated the paths.
+    return Arrays.equals(updatedAnnouncementData, bytes);


note: this seems like an exception or true

currentBytes =computeIfAbsent(path, () -> bytes) if(Arrays.equals(oldBytes, bytes)) { return; } throw ...

GWphua · 2025-03-17T10:40:33Z

Hi @kgyrtkirk,

I have made changes according to your suggestions.
I also took the suggestion of @cryptoe and created a feature flag to allow users to choose between the two flavours of announcers currently available. The default is set to using the old PathChildrenCache Announcer, and users can opt in for the new NodeCache Announcer should they face problems with huge ZooKeeper watch count.

The PR description is edited accordingly.

asdf2014 and others added 16 commits January 13, 2020 14:33

Patch comments

8420d73

Add license

da8538b

Patch comments

67b5064

Merge branch 'master' into tmp

23cd4d8

Merge reduce ZK

ce64445

Update to use CloseableUtils instead of CloseQuietly.

da2f212

Add Javadocs for Announceable class

a5d0669

Javadocs on Announcers

f3b21a4

Refactor announce method

1053042

Refactor NodeAnnouncer

903db88

Add Unit Tests

954fa53

Remove humor from logs

25de238

Add unit test for NodeAnnouncer announcing same path different payload.

a7733e3

Merge branch 'apache:master' into zk-fix

31f1d3d

Tweak import and change Javadocs

f541b84

github-actions bot added the Area - Ingestion label Nov 15, 2024

GWphua added 3 commits November 18, 2024 09:54

Update Druid unannouncement logging to make sense

20cfb0f

Replace deprecated inTransaction() with transaction()

2a1f803

Refactor buildParentPath

a857bfc

GWphua marked this pull request as draft November 21, 2024 01:24

GWphua added 9 commits November 21, 2024 14:11

Merge branch 'github' into zk-fix

1cbfa99

Merge branch 'github' into zk-fix

e50e2aa

Add executorService for NodeCache

582691e

Try replace all Announcer

a305748

Add JavaDocs

438bfd9

Refactor Test classes

c6bfbd6

Add JavaDocs and logging messages

1aea321

Fix Checkstyle

6e262f8

Add debug logs for update

b412415

Replace CloseableExecutorService within NodeCaches with ExecutorService

6c9bb94

GWphua marked this pull request as ready for review December 3, 2024 03:34

GWphua added 3 commits December 3, 2024 14:59

Debug logs for NodeAnnouncer starting and stopping

c8636d2

Add debug logs for Announcer.java

f12e09d

Merge branch 'github' into zk-fix

8d3cd1c

Merge branch 'github' into zk-fix

eb350fb

asdf2014 requested a review from gianm December 20, 2024 09:13

kgyrtkirk reviewed Feb 26, 2025

View reviewed changes

GWphua added 15 commits February 27, 2025 11:27

Merge remote-tracking branch 'origin/master' into zk-fix

cf7c45e

Remove ZKPathsUtils

64bce5c

Remove usage of ZKPathUtils in NodeAnnouncer.java

431f7b5

Remove ZKPathUtils usage in test classes

c58d8a5

Migrate unit tests away from ZkPathUtils

1a5f663

Use computeIfAbsent where necessary

948d721

Use JUnit5 for new Test class

f964497

Fix checkstyle

6f6b12d

Revert NodeAnnouncer changes

d27e27e

Fix compile

9648155

Upgrade to CuratorCache

a0513b9

Revert logging of exceptions

c3d89fc

Changes to avoid race conditions when announcing

384849c

Feature flag for announcers

7a9e09f

Checkstyle

5e67ed2

github-actions bot added the Area - Documentation label Mar 17, 2025

Dependency injection for Announcers

c0fc475

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Huge Number of Watches in ZooKeeper #17482

Fix Huge Number of Watches in ZooKeeper #17482

GWphua commented Nov 15, 2024 •

edited

Loading

GWphua commented Dec 11, 2024 •

edited

Loading

GWphua commented Dec 19, 2024

cryptoe commented Feb 10, 2025

GWphua commented Feb 21, 2025 •

edited

Loading

kgyrtkirk Feb 26, 2025

GWphua Mar 17, 2025

kgyrtkirk Feb 26, 2025

GWphua Mar 17, 2025

kgyrtkirk Feb 26, 2025

GWphua Mar 17, 2025

kgyrtkirk Feb 26, 2025

GWphua Mar 17, 2025

kgyrtkirk Feb 26, 2025

GWphua Mar 17, 2025

kgyrtkirk Feb 26, 2025

GWphua Mar 17, 2025

kgyrtkirk Feb 26, 2025

GWphua Mar 17, 2025

kgyrtkirk Feb 26, 2025

GWphua Mar 17, 2025

GWphua commented Mar 17, 2025

Fix Huge Number of Watches in ZooKeeper #17482

Are you sure you want to change the base?

Fix Huge Number of Watches in ZooKeeper #17482

Conversation

GWphua commented Nov 15, 2024 • edited Loading

Description

Fixed Huge Number of Watches in ZooKeeper

Documentation

Refactoring

Release note

Key changed/added classes in this PR

GWphua commented Dec 11, 2024 • edited Loading

GWphua commented Dec 19, 2024

Ingestion

Querying

cryptoe commented Feb 10, 2025

GWphua commented Feb 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GWphua commented Mar 17, 2025

GWphua commented Nov 15, 2024 •

edited

Loading

GWphua commented Dec 11, 2024 •

edited

Loading

GWphua commented Feb 21, 2025 •

edited

Loading