ctubbsii commented on code in PR #5350:
URL: https://github.com/apache/accumulo/pull/5350#discussion_r1999472876
##########
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:
@meatballspaghetti added a more verbose comment to explain that ZRECOVERY
was a partial path. However, I reverted it because this log is not particularly
unique... we have lots of logs that are going to display paths from ZK, and we
don't want to explain that on every one, and trying to figure out which ones
should have it and which ones shouldn't is going to be very tedious... and
might require passing around the instanceId or root path again, just for log
messages. I think the shorter paths are going to take some getting used to, but
I don't think it's worth keeping around all the extra code passing around these
things just for log messages.
--
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]
