dlmarion commented on code in PR #5350:
URL: https://github.com/apache/accumulo/pull/5350#discussion_r1992107291
##########
server/base/src/main/java/org/apache/accumulo/server/conf/util/ZooPropEditor.java:
##########
@@ -180,7 +179,8 @@ private void printProperties(final ServerContext context,
final PropStoreKey<?>
writer.printf(": Property scope: %s\n", scope);
writer.printf(": ZooKeeper path: %s\n", propKey.getPath());
writer.printf(": Name: %s\n", getDisplayName(propKey, context));
- writer.printf(": Id: %s\n", propKey.getId());
+ writer.printf(": Id: %s\n", propKey instanceof IdBasedPropStoreKey
+ ? ((IdBasedPropStoreKey<?>) propKey).getId() : "N/A");
Review Comment:
Why print "N/A" here? Could print `propKey.canonical()`, right?
##########
server/base/src/main/java/org/apache/accumulo/server/conf/store/impl/ZooPropStore.java:
##########
@@ -85,33 +83,28 @@ public class ZooPropStore implements PropStore,
PropChangeListener {
} else {
this.cache = new
PropCacheCaffeineImpl.Builder(propLoader).forTests(ticker).build();
}
-
try {
- var path = ZooUtil.getRoot(instanceId);
- if (zrw.exists(path, propStoreWatcher)) {
- log.debug("Have a ZooKeeper connection and found instance node: {}",
instanceId);
+ if (zrw.exists("/", propStoreWatcher)) {
+ log.debug("Have a ZooKeeper connection and found instance node: {}");
zkReadyMon.setReady();
} else {
- throw new IllegalStateException("Instance may not have been
initialized, root node: " + path
- + " does not exist in ZooKeeper");
+ throw new IllegalStateException(
+ "Instance may not have been initialized, provided root node path
does not exist in ZooKeeper");
}
} catch (InterruptedException ex) {
Thread.currentThread().interrupt();
- throw new IllegalStateException(
- "Interrupted trying to read root node " + instanceId + " from
ZooKeeper", ex);
+ throw new IllegalStateException("Interrupted trying to read root node
from ZooKeeper", ex);
} catch (KeeperException ex) {
- throw new IllegalStateException("Failed to read root node " + instanceId
+ " from ZooKeeper",
- ex);
+ throw new IllegalStateException("Failed to read root node from
ZooKeeper", ex);
Review Comment:
Same comment applies here about the root node.
##########
server/base/src/main/java/org/apache/accumulo/server/conf/store/impl/ZooPropStore.java:
##########
@@ -85,33 +83,28 @@ public class ZooPropStore implements PropStore,
PropChangeListener {
} else {
this.cache = new
PropCacheCaffeineImpl.Builder(propLoader).forTests(ticker).build();
}
-
try {
- var path = ZooUtil.getRoot(instanceId);
- if (zrw.exists(path, propStoreWatcher)) {
- log.debug("Have a ZooKeeper connection and found instance node: {}",
instanceId);
+ if (zrw.exists("/", propStoreWatcher)) {
+ log.debug("Have a ZooKeeper connection and found instance node: {}");
zkReadyMon.setReady();
} else {
- throw new IllegalStateException("Instance may not have been
initialized, root node: " + path
- + " does not exist in ZooKeeper");
+ throw new IllegalStateException(
+ "Instance may not have been initialized, provided root node path
does not exist in ZooKeeper");
}
} catch (InterruptedException ex) {
Thread.currentThread().interrupt();
- throw new IllegalStateException(
- "Interrupted trying to read root node " + instanceId + " from
ZooKeeper", ex);
+ throw new IllegalStateException("Interrupted trying to read root node
from ZooKeeper", ex);
Review Comment:
This message is a little misleading. Root node with no context means '/',
when in reality you can't read the instance root node which is at
`/accumulo/<uuid>`.
##########
server/manager/src/main/java/org/apache/accumulo/manager/Manager.java:
##########
@@ -1288,20 +1284,20 @@ public void run() {
ZooReaderWriter zReaderWriter = context.getZooSession().asReaderWriter();
try {
- zReaderWriter.getChildren(zroot + Constants.ZRECOVERY, new Watcher() {
+ zReaderWriter.getChildren(Constants.ZRECOVERY, new Watcher() {
@Override
public void process(WatchedEvent event) {
nextEvent.event("Noticed recovery changes %s", event.getType());
try {
// watcher only fires once, add it back
- zReaderWriter.getChildren(zroot + Constants.ZRECOVERY, this);
+ zReaderWriter.getChildren(Constants.ZRECOVERY, this);
} catch (Exception e) {
log.error("Failed to add log recovery watcher back", e);
}
}
});
} catch (KeeperException | InterruptedException e) {
- throw new IllegalStateException("Unable to read " + zroot +
Constants.ZRECOVERY, e);
+ throw new IllegalStateException("Unable to read " + Constants.ZRECOVERY,
e);
Review Comment:
Same comment here about the path in zookeeper not being entirely correct in
the log or error message.
##########
server/base/src/main/java/org/apache/accumulo/server/conf/store/impl/ZooPropStore.java:
##########
@@ -85,33 +83,28 @@ public class ZooPropStore implements PropStore,
PropChangeListener {
} else {
this.cache = new
PropCacheCaffeineImpl.Builder(propLoader).forTests(ticker).build();
}
-
try {
- var path = ZooUtil.getRoot(instanceId);
- if (zrw.exists(path, propStoreWatcher)) {
- log.debug("Have a ZooKeeper connection and found instance node: {}",
instanceId);
+ if (zrw.exists("/", propStoreWatcher)) {
Review Comment:
"/" has me wondering after this change in how many places Constants.ZROOT is
used. If localized to one class to get the instanceId, then maybe
Constants.ZROOT should be removed or moved into that class. Maybe '/' should
become `Constants.ZINSTANCE_ROOT`.
--
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]
