keith-turner commented on code in PR #5256:
URL: https://github.com/apache/accumulo/pull/5256#discussion_r1929030328
##########
core/src/main/java/org/apache/accumulo/core/zookeeper/ZooCache.java:
##########
@@ -391,44 +425,27 @@ public byte[] run() throws KeeperException,
InterruptedException {
if (zcn != null && zcn.cachedData()) {
return zcn;
}
- /*
- * The following call to exists() is important, since we are caching
that a node does not
- * exist. Once the node comes into existence, it will be added to
the cache. But this
- * notification of a node coming into existence will only be given
if exists() was
- * previously called. If the call to exists() is bypassed and only
getData() is called
- * with a special case that looks for Code.NONODE in the
KeeperException, then
- * non-existence can not be cached.
- */
try {
- Stat stat = zk.exists(zPath, watcher);
- if (stat == null) {
- if (log.isTraceEnabled()) {
- log.trace("{} zookeeper did not contain {}", cacheId, zPath);
- }
+ byte[] data = null;
+ Stat stat = new Stat();
+ ZcStat zstat = null;
+ try {
+ data = zk.getData(zPath, null, stat);
+ zstat = new ZcStat(stat);
+ } catch (KeeperException.BadVersionException |
KeeperException.NoNodeException e1) {
Review Comment:
Should no longer see the BadVersionException. In the previous code that was
seen when the node changed between the zk.exists() and zk.getData() calls.
```suggestion
} catch (KeeperException.NoNodeException e1) {
```
##########
core/src/test/java/org/apache/accumulo/core/zookeeper/ZooCacheTest.java:
##########
@@ -16,86 +16,118 @@
* specific language governing permissions and limitations
* under the License.
*/
-package org.apache.accumulo.core.fate.zookeeper;
+package org.apache.accumulo.core.zookeeper;
-import static org.easymock.EasyMock.anyObject;
-import static org.easymock.EasyMock.capture;
import static org.easymock.EasyMock.createStrictMock;
-import static org.easymock.EasyMock.eq;
import static org.easymock.EasyMock.expect;
+import static org.easymock.EasyMock.isA;
import static org.easymock.EasyMock.replay;
import static org.easymock.EasyMock.verify;
import static org.junit.jupiter.api.Assertions.assertArrayEquals;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertNull;
import static org.junit.jupiter.api.Assertions.assertSame;
+import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.assertTrue;
import java.util.List;
+import java.util.Set;
+import java.util.UUID;
-import org.apache.accumulo.core.fate.zookeeper.ZooCache.ZcStat;
-import org.apache.accumulo.core.fate.zookeeper.ZooCache.ZooCacheWatcher;
-import org.apache.accumulo.core.zookeeper.ZooSession;
+import org.apache.accumulo.core.Constants;
+import org.apache.accumulo.core.zookeeper.ZooCache.ZooCacheWatcher;
import org.apache.zookeeper.KeeperException;
import org.apache.zookeeper.WatchedEvent;
import org.apache.zookeeper.Watcher;
import org.apache.zookeeper.data.Stat;
-import org.easymock.Capture;
-import org.easymock.EasyMock;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
public class ZooCacheTest {
- private static final String ZPATH = "/some/path/in/zk";
+
+ /**
+ * Test class that extends ZooCache to suppress the creation of the
persistent recursive watchers
+ * that are created in the constructor and to provide access to the watcher.
+ */
+ private static class TestZooCache extends ZooCache {
+
+ public TestZooCache(ZooSession zk, Set<String> pathsToWatch) {
+ super(zk, pathsToWatch);
+ }
+
+ @Override
+ protected void setupWatchers() {
+ clear();
+ }
+
+ public void executeWatcher(WatchedEvent event) {
+ // simulate ZooKeeper calling our Watcher
+ watcher.process(event);
+ }
+
+ @Override
+ long getZKClientObjectVersion() {
+ return 1L;
+ }
+
+ }
+
+ private static final String root =
+ Constants.ZROOT + "/" + UUID.randomUUID().toString() +
Constants.ZTSERVERS;
+ private static final String ZPATH = root + "/testPath";
private static final byte[] DATA = {(byte) 1, (byte) 2, (byte) 3, (byte) 4};
private static final List<String> CHILDREN = java.util.Arrays.asList("huey",
"dewey", "louie");
private ZooSession zk;
- private ZooCache zc;
+ private TestZooCache zc;
@BeforeEach
public void setUp() {
zk = createStrictMock(ZooSession.class);
- zc = new ZooCache(zk);
+ zc = new TestZooCache(zk, Set.of(root));
}
+ @SuppressWarnings("unchecked")
@Test
- public void testGet() throws Exception {
- testGet(false);
+ public void testOverlappingPaths() throws Exception {
+ expect(zk.getConnectionCounter()).andReturn(2L).times(2);
+ zk.addPersistentRecursiveWatchers(isA(Set.class), isA(Watcher.class));
+ replay(zk);
+ assertThrows(IllegalArgumentException.class,
+ () -> new ZooCache(zk, Set.of(root, root + "/localhost:9995")));
+
+ Set<String> goodPaths =
Set.of("/accumulo/8247eee6-a176-4e19-baf7-e3da965fe050/compactors",
+ "/accumulo/8247eee6-a176-4e19-baf7-e3da965fe050/dead/tservers",
+ "/accumulo/8247eee6-a176-4e19-baf7-e3da965fe050/gc/lock",
+ "/accumulo/8247eee6-a176-4e19-baf7-e3da965fe050/managers/lock",
+ "/accumulo/8247eee6-a176-4e19-baf7-e3da965fe050/namespaces",
+ "/accumulo/8247eee6-a176-4e19-baf7-e3da965fe050/recovery",
+ "/accumulo/8247eee6-a176-4e19-baf7-e3da965fe050/root_tablet",
+ "/accumulo/8247eee6-a176-4e19-baf7-e3da965fe050/sservers",
+ "/accumulo/8247eee6-a176-4e19-baf7-e3da965fe050/tables",
+ "/accumulo/8247eee6-a176-4e19-baf7-e3da965fe050/tservers",
+ "/accumulo/8247eee6-a176-4e19-baf7-e3da965fe050/users",
+ "/accumulo/8247eee6-a176-4e19-baf7-e3da965fe050/mini",
+ "/accumulo/8247eee6-a176-4e19-baf7-e3da965fe050/monitor/lock");
+ new ZooCache(zk, goodPaths);
+ verify(zk);
Review Comment:
Could add test for trying to getData and getChildren from zoocache for paths
that are not watched and check that an exception is thrown.
##########
core/src/main/java/org/apache/accumulo/core/zookeeper/ZooCache.java:
##########
@@ -143,65 +141,109 @@ public void process(WatchedEvent event) {
clear();
break;
default:
- log.warn("{} Unhandled {}", cacheId, event);
+ log.warn("{} Unhandled state {}", cacheId, event);
break;
}
break;
default:
- log.warn("{} Unhandled {}", cacheId, event);
+ log.warn("{} Unhandled event type {}", cacheId, event);
break;
}
- externalWatcher.ifPresent(w -> w.accept(event));
+ externalWatchers.forEach(ew -> ew.accept(event));
}
}
/**
- * Creates a new cache without an external watcher.
+ * Creates a ZooCache instance that uses the supplied ZooSession for
communicating with the
+ * instance's ZooKeeper servers. The ZooCache will create persistent
watchers at the given
+ * pathsToWatch, if any, to be updated when changes are made in ZooKeeper
for nodes at or below in
+ * the tree. If ZooCacheWatcher's are added via {@code addZooCacheWatcher},
then they will be
+ * notified when this object is notified of changes via the
PersistentWatcher callback.
*
- * @param zk the ZooKeeper instance
- * @throws NullPointerException if zk is {@code null}
+ * @param zk ZooSession for this instance
+ * @param pathsToWatch Paths in ZooKeeper to watch
*/
- public ZooCache(ZooSession zk) {
- this(zk, Optional.empty(), Duration.ofMinutes(3));
+ public ZooCache(ZooSession zk, Set<String> pathsToWatch) {
+ this(zk, pathsToWatch, Ticker.systemTicker());
+ }
+
+ // visible for tests that use a Ticker
+ public ZooCache(ZooSession zk, Set<String> pathsToWatch, Ticker ticker) {
+ this.zk = requireNonNull(zk);
+ this.zkClientTracker.set(this.getZKClientObjectVersion());
+ this.cache =
Caches.getInstance().createNewBuilder(Caches.CacheName.ZOO_CACHE, false)
+
.ticker(requireNonNull(ticker)).expireAfterAccess(CACHE_DURATION).build();
+ // The concurrent map returned by Caffeine will only allow one thread to
run at a time for a
+ // given key and ZooCache relies on that. Not all concurrent map
implementations have this
+ // behavior for their compute functions.
+ this.nodeCache = cache.asMap();
+ this.watchedPaths = Collections.unmodifiableNavigableSet(new
TreeSet<>(pathsToWatch));
+ setupWatchers();
+ log.trace("{} created new cache watching {}", cacheId, pathsToWatch, new
Exception());
+ }
+
+ public void addZooCacheWatcher(ZooCacheWatcher watcher) {
Review Comment:
Looked in the code to see what uses this and found TabletManager which has
handling for `NodeChildrenChanged` which seems like its unexpected now. AFAICT
the code in TableManager seems correct w/ the new watch behavior because it has
handling for `NodeCreated` and `NodeDeleted. Wondering if its handling for
`NodeChildrenChanged` could be removed.
##########
core/src/main/java/org/apache/accumulo/core/zookeeper/ZooCache.java:
##########
@@ -391,44 +425,27 @@ public byte[] run() throws KeeperException,
InterruptedException {
if (zcn != null && zcn.cachedData()) {
return zcn;
}
- /*
- * The following call to exists() is important, since we are caching
that a node does not
- * exist. Once the node comes into existence, it will be added to
the cache. But this
- * notification of a node coming into existence will only be given
if exists() was
- * previously called. If the call to exists() is bypassed and only
getData() is called
- * with a special case that looks for Code.NONODE in the
KeeperException, then
- * non-existence can not be cached.
- */
try {
- Stat stat = zk.exists(zPath, watcher);
- if (stat == null) {
- if (log.isTraceEnabled()) {
- log.trace("{} zookeeper did not contain {}", cacheId, zPath);
- }
+ byte[] data = null;
+ Stat stat = new Stat();
+ ZcStat zstat = null;
+ try {
+ data = zk.getData(zPath, null, stat);
+ zstat = new ZcStat(stat);
+ } catch (KeeperException.BadVersionException |
KeeperException.NoNodeException e1) {
+ log.trace("{} zookeeper did not contain {}", cacheId, zPath);
return ZcNode.NON_EXISTENT;
- } else {
- byte[] data = null;
- ZcStat zstat = null;
- try {
- data = zk.getData(zPath, watcher, stat);
- zstat = new ZcStat(stat);
- } catch (KeeperException.BadVersionException |
KeeperException.NoNodeException e1) {
- throw new ConcurrentModificationException(e1);
Review Comment:
The retry code catches this exception. Now that this code is no longer
throwing it can remove that catch in the retry code.
##########
server/base/src/main/java/org/apache/accumulo/server/security/handler/ZKPermHandler.java:
##########
@@ -272,11 +268,9 @@ public void revokeSystemPermission(String user,
SystemPermission permission)
try {
if (sysPerms.remove(permission)) {
- synchronized (zooCache) {
- zooCache.clear();
- zoo.putPersistentData(zkUserPath + "/" + user + ZKUserSysPerms,
- ZKSecurityTool.convertSystemPermissions(sysPerms),
NodeExistsPolicy.OVERWRITE);
- }
+ ctx.getZooCache().clear((path) -> path.startsWith(sysPermPath));
Review Comment:
Not a change for this PR, but it seems like these clears could be removed
from the code. The cache should eventually update as an event will be
generated from the zookeeper update. Its nice to put all the paths that used
more than once in a variable in this PR.
--
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]
