ctubbsii commented on code in PR #5256:
URL: https://github.com/apache/accumulo/pull/5256#discussion_r1924410980
##########
core/src/main/java/org/apache/accumulo/core/fate/zookeeper/ZooCache.java:
##########
@@ -46,30 +52,44 @@
import org.slf4j.LoggerFactory;
import com.github.benmanes.caffeine.cache.Cache;
-import com.github.benmanes.caffeine.cache.RemovalListener;
+import com.github.benmanes.caffeine.cache.Ticker;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Preconditions;
/**
* A cache for values stored in ZooKeeper. Values are kept up to date as they
change.
*/
-public class ZooCache {
+public class ZooCache implements Watcher {
Review Comment:
I think making ZooCache implement watcher tends to create confusion. I think
it's better if ZooCache *contains* a watcher (that serves its needs) rather
than *is* a watcher.
##########
core/src/main/java/org/apache/accumulo/core/fate/zookeeper/ZooCache.java:
##########
@@ -554,4 +616,32 @@ public Optional<ServiceLockData>
getLockData(ServiceLockPath path) {
return ServiceLockData.parse(lockData);
}
+ @SuppressWarnings("deprecation")
+ @Override
+ public void process(WatchedEvent event) {
+ // ZooCache implements the Watcher interface so that it can
+ // receive events from the ZooSesssion session watcher.
+ switch (event.getState()) {
+ case SyncConnected:
+ log.info("{} ZooKeeper connection established, re-establishing
watchers; {}", cacheId,
+ event);
+ clear();
+ setupWatchers(pathsToWatch);
+ break;
Review Comment:
SyncConnected doesn't just happen when establishing a new session. It also
happens when a session is migrated from one ZooKeeper server to another. This
would reset cause redundant persistent recursive watchers to be added to the
same session. That might be okay if ZooKeeper checks them to see if they've
already been added, but it seems sketchy to assume that this is a new session.
##########
core/src/main/java/org/apache/accumulo/core/zookeeper/ZooSession.java:
##########
@@ -188,6 +193,7 @@ private synchronized ZooKeeper reconnect() {
digestAuth(zk, instanceSecret);
}
tryAgain = false;
+
Review Comment:
random extra newline added
```suggestion
```
##########
core/src/main/java/org/apache/accumulo/core/fate/zookeeper/ZooCache.java:
##########
@@ -554,4 +616,32 @@ public Optional<ServiceLockData>
getLockData(ServiceLockPath path) {
return ServiceLockData.parse(lockData);
}
+ @SuppressWarnings("deprecation")
+ @Override
+ public void process(WatchedEvent event) {
+ // ZooCache implements the Watcher interface so that it can
+ // receive events from the ZooSesssion session watcher.
+ switch (event.getState()) {
+ case SyncConnected:
+ log.info("{} ZooKeeper connection established, re-establishing
watchers; {}", cacheId,
+ event);
+ clear();
+ setupWatchers(pathsToWatch);
+ break;
+ case Closed:
+ case Disconnected:
+ this.watchersSet.set(false);
+ break;
+ case Expired:
+ case AuthFailed:
+ case ConnectedReadOnly:
+ case NoSyncConnected:
+ case SaslAuthenticated:
+ case Unknown:
+ default:
+ break;
Review Comment:
redundant
```suggestion
default:
break;
```
--
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: notifications-unsubscr...@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
