keith-turner commented on code in PR #5040:
URL: https://github.com/apache/accumulo/pull/5040#discussion_r1834568546
##########
core/src/main/java/org/apache/accumulo/core/fate/zookeeper/ZooCache.java:
##########
@@ -294,41 +296,46 @@ public List<String> getChildren(final String zPath) {
@Override
public List<String> run() throws KeeperException, InterruptedException {
- // only read volatile once for consistency
- ImmutableCacheCopies lic = immutableCache;
- var cachedChildren = lic.childrenCache.get(zPath);
- // null is cached as a value, that is the reason for the secondary
containsKey check
- if (cachedChildren != null || lic.childrenCache.containsKey(zPath)) {
- return cachedChildren;
+ var zcNode = nodeCache.get(zPath);
+ if (zcNode != null && zcNode.childrenSet) {
+ return zcNode.children;
}
- cacheWriteLock.lock();
try {
- List<String> children = childrenCache.get(zPath);
- // null is cached as a value, that is the reason for the secondary
containsKey check
- if (children != null || lic.childrenCache.containsKey(zPath)) {
- return children;
- }
-
- final ZooKeeper zooKeeper = getZooKeeper();
+ zcNode = nodeCache.compute(zPath, (zp, zcn) -> {
+ // recheck the children now that lock is held on key
+ if (zcn != null && zcn.childrenSet) {
+ return zcn;
+ }
- children = zooKeeper.getChildren(zPath, watcher);
- if (children != null) {
- children = List.copyOf(children);
- }
- childrenCache.put(zPath, children);
- immutableCache = new ImmutableCacheCopies(++updateCount,
immutableCache, childrenCache);
- return children;
- } catch (KeeperException ke) {
- if (ke.code() != Code.NONODE) {
- throw ke;
+ try {
+ final ZooKeeper zooKeeper = getZooKeeper();
+ List<String> children = null;
+ // TODO zookeeper may never return null
Review Comment:
I need to look into this. I think the code is assuming that when a node
does not exist that null is returned. However when a node does not exist the
javadocs say that a KeeperException.NONODE is thrown. So I think the code is
attempting to cache non-existence but is not actually doing that. I need to
remove this todo and open an issue prior to merging.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
