ctubbsii commented on code in PR #5256:
URL: https://github.com/apache/accumulo/pull/5256#discussion_r1915416609
##########
core/src/main/java/org/apache/accumulo/core/fate/zookeeper/ZooCache.java:
##########
@@ -59,16 +66,39 @@ public interface ZooCacheWatcher extends
Consumer<WatchedEvent> {}
private static final Logger log = LoggerFactory.getLogger(ZooCache.class);
- private final ZCacheWatcher watcher = new ZCacheWatcher();
- private final Optional<ZooCacheWatcher> externalWatcher;
+ protected static final String[] ALLOWED_PATHS = new String[]
{Constants.ZCOMPACTORS,
+ Constants.ZDEADTSERVERS, Constants.ZGC_LOCK, Constants.ZMANAGER_LOCK,
Constants.ZMINI_LOCK,
+ Constants.ZMONITOR_LOCK, Constants.ZNAMESPACES, Constants.ZRECOVERY,
Constants.ZSSERVERS,
+ Constants.ZTABLES, Constants.ZTSERVERS, Constants.ZUSERS,
RootTable.ZROOT_TABLET};
+
+ protected final TreeSet<String> watchedPaths = new TreeSet<>();
+ // visible for tests
+ protected final ZCacheWatcher watcher = new ZCacheWatcher();
+ private final List<ZooCacheWatcher> externalWatchers =
+ Collections.synchronizedList(new ArrayList<>());
private static final AtomicLong nextCacheId = new AtomicLong(0);
private final String cacheId = "ZC" + nextCacheId.incrementAndGet();
+ public static final Duration CACHE_DURATION = Duration.ofMinutes(30);
+
+ // public and non-final because this is being set
+ // in tests to test the eviction
+ @SuppressFBWarnings(value = "MS_SHOULD_BE_FINAL",
+ justification = "being set in tests for eviction test")
+ public static Ticker ticker = Ticker.systemTicker();
+
+ // Construct this here, otherwise end up with NPE in some cases
+ // when the Watcher tries to access nodeCache. Alternative would
+ // be to mark nodeCache as volatile.
+ private final Cache<String,ZcNode> cache =
+ Caches.getInstance().createNewBuilder(Caches.CacheName.ZOO_CACHE,
false).ticker(ticker)
+ .expireAfterAccess(CACHE_DURATION).build();
Review Comment:
Could also memoize it so it is lazily constructed on first use.
##########
core/src/main/java/org/apache/accumulo/core/fate/zookeeper/ZooCache.java:
##########
@@ -59,16 +66,39 @@ public interface ZooCacheWatcher extends
Consumer<WatchedEvent> {}
private static final Logger log = LoggerFactory.getLogger(ZooCache.class);
- private final ZCacheWatcher watcher = new ZCacheWatcher();
- private final Optional<ZooCacheWatcher> externalWatcher;
+ protected static final String[] ALLOWED_PATHS = new String[]
{Constants.ZCOMPACTORS,
+ Constants.ZDEADTSERVERS, Constants.ZGC_LOCK, Constants.ZMANAGER_LOCK,
Constants.ZMINI_LOCK,
+ Constants.ZMONITOR_LOCK, Constants.ZNAMESPACES, Constants.ZRECOVERY,
Constants.ZSSERVERS,
+ Constants.ZTABLES, Constants.ZTSERVERS, Constants.ZUSERS,
RootTable.ZROOT_TABLET};
+
+ protected final TreeSet<String> watchedPaths = new TreeSet<>();
+ // visible for tests
+ protected final ZCacheWatcher watcher = new ZCacheWatcher();
+ private final List<ZooCacheWatcher> externalWatchers =
+ Collections.synchronizedList(new ArrayList<>());
private static final AtomicLong nextCacheId = new AtomicLong(0);
private final String cacheId = "ZC" + nextCacheId.incrementAndGet();
+ public static final Duration CACHE_DURATION = Duration.ofMinutes(30);
+
+ // public and non-final because this is being set
+ // in tests to test the eviction
+ @SuppressFBWarnings(value = "MS_SHOULD_BE_FINAL",
+ justification = "being set in tests for eviction test")
+ public static Ticker ticker = Ticker.systemTicker();
Review Comment:
Instead of doing this, could put the ticker in a protected method that the
tests can override when they subclass it for testing.
##########
core/src/main/java/org/apache/accumulo/core/fate/zookeeper/ZooCache.java:
##########
@@ -120,7 +150,14 @@ public void process(WatchedEvent event) {
case NodeDeleted:
case ChildWatchRemoved:
case DataWatchRemoved:
- remove(event.getPath());
+ // This code use to call remove(path), but that was when a Watcher
was set
+ // on each node. With the Watcher being set at a higher level we
need to remove
+ // the parent of the affected node and all of its children from the
cache
+ // so that the parent and children node can be re-cached. If we only
remove the
+ // affected node, then the cached children in the parent could be
incorrect.
Review Comment:
It's shorter and simpler to document what the code currently does and why,
rather than explain what it used to do. That part can be removed from the
comment.
Previously, the code tried to handle several different cases at the same
time, but it seems to me that we may want to handle some of those cases
differently now with a persistent watcher.
One thing I don't know, that comments here could help make clear is whether
the path in the event is the root path where the persistent watcher is set, or
if it's the actual node affected. That would help the reader understand why the
code is doing what it is doing.
##########
core/src/main/java/org/apache/accumulo/core/fate/zookeeper/ZooCache.java:
##########
@@ -152,53 +189,53 @@ public void process(WatchedEvent event) {
break;
}
- externalWatcher.ifPresent(w -> w.accept(event));
+ externalWatchers.forEach(ew -> ew.accept(event));
}
}
- /**
- * Creates a new cache without an external watcher.
- *
- * @param zk the ZooKeeper instance
- * @throws NullPointerException if zk is {@code null}
- */
- public ZooCache(ZooSession zk) {
- this(zk, Optional.empty(), Duration.ofMinutes(3));
+ public ZooCache(ZooSession zk, String root, ZooCacheWatcher... watchers) {
+ this.zk = requireNonNull(zk);
+ for (ZooCacheWatcher zcw : watchers) {
+ externalWatchers.add(zcw);
+ }
+ setupWatchers(requireNonNull(root));
+ log.trace("{} created new cache", cacheId, new Exception());
}
- /**
- * Creates a new cache. The given watcher is called whenever a watched node
changes.
- *
- * @param zk the ZooKeeper instance
- * @param watcher watcher object
- * @throws NullPointerException if zk or watcher is {@code null}
- */
- public ZooCache(ZooSession zk, ZooCacheWatcher watcher) {
- this(zk, Optional.of(watcher), Duration.ofMinutes(3));
+ public void addZooCacheWatcher(ZooCacheWatcher watcher) {
+ externalWatchers.add(requireNonNull(watcher));
Review Comment:
Do we need to be able to provide several, or to add them later? This
introduces a lot of complexity into the ZooCache logic that could be left
outside of ZooCache. For instance, the user can provide a more complex external
watcher rather than several simpler watchers to trigger one at a time. Callers
have never needed more than one external watcher before, so I'm not sure they
need to have more now.
##########
core/src/main/java/org/apache/accumulo/core/fate/zookeeper/ZooCache.java:
##########
@@ -106,11 +136,11 @@ public long getMzxid() {
private final AtomicLong updateCount = new AtomicLong(0);
- private class ZCacheWatcher implements Watcher {
+ class ZCacheWatcher implements Watcher {
@Override
public void process(WatchedEvent event) {
if (log.isTraceEnabled()) {
- log.trace("{}: {}", cacheId, event);
+ log.trace("{} {}: {}", cacheId, event.getType(), event);
Review Comment:
Isn't the type already included in the event.toString()?
##########
core/src/main/java/org/apache/accumulo/core/fate/zookeeper/ZooCache.java:
##########
@@ -59,16 +64,38 @@ public interface ZooCacheWatcher extends
Consumer<WatchedEvent> {}
private static final Logger log = LoggerFactory.getLogger(ZooCache.class);
- private final ZCacheWatcher watcher = new ZCacheWatcher();
+ protected static final String[] ALLOWED_PATHS = new String[]
{Constants.ZCOMPACTORS,
+ Constants.ZDEADTSERVERS, Constants.ZGC_LOCK, Constants.ZMANAGER_LOCK,
Constants.ZMINI_LOCK,
+ Constants.ZMONITOR_LOCK, Constants.ZNAMESPACES, Constants.ZRECOVERY,
Constants.ZSSERVERS,
+ Constants.ZTABLES, Constants.ZTSERVERS, Constants.ZUSERS,
RootTable.ZROOT_TABLET};
Review Comment:
I don't think this should be hard-coded into ZooCache. It changes ZooCache
from a generic caching utility into one specifically tailored for these paths,
making it harder to reuse elsewhere. If it is to watch specific paths, then
those paths should be provided when it is constructed. That would keep ZooCache
simpler, and more flexible for reuse.
--
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]
