New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Lattice Cache - Caching Just Data Implementation #2619

Merged

xyuanlu merged 6 commits into apache:metaclient from Marcosrico:dataCache

Sep 28, 2023

Contributor

Marcosrico commented Sep 19, 2023 •

edited

Loading

Issues

My PR addresses the following Helix issues and references them in the PR description:

Description

Here are some details about my PR, including screenshots of any UI changes:

This PR includes all the implementation of the dataCache Map for storing the data in cache as well as the implementation of data specific apis (get and batch get).
Also made some refactoring for config builder.

Tests

The following tests are written for this issue:

testLazyDataCacheAndFetch and testCacheDataUpdates

Commits

My commits all reference appropriate Apache Helix GitHub issues in their subject lines. In addition, my commits follow the guidelines from "How to write a good git commit message":
1. Subject is separated from body by a blank line
2. Subject is limited to 50 characters (not including Jira issue reference)
3. Subject does not end with a period
4. Subject uses the imperative mood ("add", not "adding")
5. Body wraps at 72 characters
6. Body explains "what" and "why", not "how"

Code Quality

My diff has been formatted using helix-style.xml
(helix-style-intellij.xml if IntelliJ IDE is used)

mapeng added 2 commits

September 18, 2023 22:00


          Data cache implementation

e017b05


          Data cache

0b47a24

Marcosrico marked this pull request as ready for review

September 19, 2023 05:06

xyuanlu reviewed

View reviewed changes

meta-client/src/main/java/org/apache/helix/metaclient/impl/zk/ZkMetaClientCache.java Outdated Show resolved Hide resolved

xyuanlu reviewed

View reviewed changes

meta-client/src/main/java/org/apache/helix/metaclient/impl/zk/ZkMetaClientCache.java Outdated Show resolved Hide resolved


          Separate thread for event changes. Hasmap computeifabsent

bfeca3b

xyuanlu reviewed

View reviewed changes

meta-client/src/main/java/org/apache/helix/metaclient/impl/zk/ZkMetaClientCache.java Outdated Show resolved Hide resolved


          Remove lazy caching. Prepopulate in the beginning, return null if dat…

276f3b1

…a doesn't exist.

xyuanlu reviewed

View reviewed changes

meta-client/src/main/java/org/apache/helix/metaclient/impl/zk/ZkMetaClientCache.java Outdated Show resolved Hide resolved

xyuanlu reviewed

View reviewed changes

meta-client/src/main/java/org/apache/helix/metaclient/impl/zk/ZkMetaClientCache.java Outdated

                   @Override
                   public T get(final String key) {
                       if (_cacheData) {
-                          getDataCacheMap().computeIfAbsent(key, k -> _cacheClient.readData(k, true));
                           return getDataCacheMap().get(key);

Contributor

xyuanlu Sep 24, 2023

shall we differentiate if key is not in ZK, or it is in ZK but not populated to cache yet?

Contributor Author

Marcosrico Sep 26, 2023

I am not too sure how to do this without pinging zookeeper. If it's not populated yet how do we know if it doesn't actually exist in zookeeper or it's being populated without checking zk?

meta-client/src/main/java/org/apache/helix/metaclient/impl/zk/ZkMetaClientCache.java

-                              dataList.add(_cacheClient.readData(key, true));
-                          }
+                      for (String key : keys) {
+                          dataList.add(get(key));

Contributor

xyuanlu Sep 24, 2023

same as above.

meta-client/src/main/java/org/apache/helix/metaclient/impl/zk/ZkMetaClientCache.java Outdated Show resolved Hide resolved

meta-client/src/main/java/org/apache/helix/metaclient/impl/zk/ZkMetaClientCache.java Outdated Show resolved Hide resolved

xyuanlu reviewed

View reviewed changes

meta-client/src/main/java/org/apache/helix/metaclient/impl/zk/ZkMetaClientCache.java Outdated Show resolved Hide resolved


          Addressed comments

7355b82

xyuanlu reviewed

View reviewed changes

meta-client/src/main/java/org/apache/helix/metaclient/impl/zk/ZkMetaClientCache.java

+                      private final String path;
+                      private final ChildChangeListener.ChangeType changeType;
+                      public CacheUpdateRunnable(String path, ChildChangeListener.ChangeType changeType) {

Contributor

xyuanlu Sep 27, 2023

nit: Add todo for dedup.

Contributor Author

Marcosrico Sep 27, 2023

Added todo a little lower :)


          Population of cache

d1f01df

xyuanlu reviewed

View reviewed changes

meta-client/src/main/java/org/apache/helix/metaclient/impl/zk/ZkMetaClientCache.java

+                      if (!_cacheClient.exists(_rootEntry)) {
+                          LOG.warn("Root entry: {} does not exist.", _rootEntry);
+                          // Let the other threads know that the cache is populated.
+                          _initializedCache.countDown();

Contributor

xyuanlu Sep 27, 2023

Why count down here?

Contributor Author

Marcosrico Sep 27, 2023

otherwise populateAllCache will return without counting down the latch so the cacheEventChange handler thread will wait indefinitely. We can have it twice or have it in the connect method (from previous impl)

xyuanlu approved these changes

View reviewed changes

Contributor

xyuanlu left a comment

LGTM. Thanks for addressing comments

Contributor Author

Marcosrico commented Sep 27, 2023

PR approved by @xyuanlu
Commit message: Lattice cache - caching just data implementation

xyuanlu merged commit c147eb4 into apache:metaclient

asfgit pushed a commit that referenced this pull request


          Lattice cache - caching just data implementation (#2619)

827ca82

Lattice cache - caching just data implementation
---------

Co-authored-by: mapeng <[email protected]>

xyuanlu added a commit that referenced this pull request


          Merge feature branch (#2647)

5f1a3f7

* MetaClientCache Part 1 - API's, configs, and builders (#2612)

MetaClientCache Part 1 - API's, configs, and builders

---------

Co-authored-by: mapeng <[email protected]>

* Skip one time listener re-register for exists for ZkClient - MetaClient usage. (#2637)

* Lattice cache - caching just data implementation (#2619)

Lattice cache - caching just data implementation
---------

Co-authored-by: mapeng <[email protected]>

* Add recursiveCreate functionality to metaclient (#2607)



Co-authored-by: Grant Palau Spencer <[email protected]>

* Lattice Children Cache Implementation(#2623)

Co-authored-by: mapeng <[email protected]>

---------

Co-authored-by: Marcos Rico Peng <[email protected]>
Co-authored-by: mapeng <[email protected]>
Co-authored-by: Grant Paláu Spencer <[email protected]>
Co-authored-by: Grant Palau Spencer <[email protected]>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet