> On Feb. 10, 2014, 11:24 p.m., kturner wrote: > > server/src/main/java/org/apache/accumulo/server/watcher/MonitorLog4jWatcher.java, > > line 120 > > <https://reviews.apache.org/r/17860/diff/2-3/?file=481201#file481201line120> > > > > should this code block be synchronized? just wondering if it should be > > mutually exclusive w/ the code that enables logging.
I don't think so because only the watcher will even trigger this block. The watcher will only re-fire after we've left this section. The log4j reloading won't trigger this method. > On Feb. 10, 2014, 11:24 p.m., kturner wrote: > > server/src/main/java/org/apache/accumulo/server/monitor/Monitor.java, line > > 595 > > <https://reviews.apache.org/r/17860/diff/2-3/?file=481200#file481200line595> > > > > Should this code be shared w/ master instead of duplicating? Probably. I noticed that there's also duplication with this same sort of watcher with the garbage collector. I'll make a follow on to consolidate these watchers across all processes. > On Feb. 10, 2014, 11:24 p.m., kturner wrote: > > server/src/main/java/org/apache/accumulo/server/monitor/Monitor.java, line > > 554 > > <https://reviews.apache.org/r/17860/diff/2-3/?file=481200#file481200line554> > > > > Have you manually tested upgrade? > > > > It may be better to put this w/ the zookeeper upgrade code in Master. > > Would make it easier when someones working on 1.7 upgrade to be aware of it > > and possibly remove it. Yes. I tested it against a 1.5.1-SNAPSHOT install I had locally that did not yet have these changes to ZooKeeper and it worked just fine. I can move it over to the Master code. I'll look at doing that to make sure it makes sense. > On Feb. 10, 2014, 11:24 p.m., kturner wrote: > > server/src/main/java/org/apache/accumulo/server/watcher/MonitorLog4jWatcher.java, > > line 116 > > <https://reviews.apache.org/r/17860/diff/3/?file=482201#file482201line116> > > > > stylistic comment, feel free to ignore. > > > > > > Could catch NoNodeException (before catching KeeperException) and put > > code in if(NONODE){} in catch block Ahh, good call. I didn't peek into the hierarchy of KeeperException. I like your suggestion. - Josh ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17860/#review34124 ----------------------------------------------------------- On Feb. 10, 2014, 10:26 p.m., Josh Elser wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/17860/ > ----------------------------------------------------------- > > (Updated Feb. 10, 2014, 10:26 p.m.) > > > Review request for accumulo. > > > Bugs: ACCUMULO-2334 > https://issues.apache.org/jira/browse/ACCUMULO-2334 > > > Repository: accumulo > > > Description > ------- > > Removes the necessity to compute and set ACCUMULO_LOG_HOST. We can accomplish > this by having the monitor advertise a host in addition to just the port and > have the other processes place a watcher on that node in zookeeper. > > This has some nice benefits to simplifying the code, removing the annoying > (IMO) ACCUMULO_LOG_HOST variable, and, when running the monitor on a random > port, the other services can auto-discover without having to update > configuration files and restart the services. The auto-discovery also has > benefit if the monitor has to be moved to a different host (or multiple > monitors are being run with "failover") > > > Diffs > ----- > > conf/examples/1GB/native-standalone/accumulo-env.sh aa4a1d0 > conf/examples/1GB/standalone/accumulo-env.sh 1707f3d > conf/examples/2GB/native-standalone/accumulo-env.sh ef74ca7 > conf/examples/2GB/standalone/accumulo-env.sh 75014c2 > conf/examples/3GB/native-standalone/accumulo-env.sh ae0da11 > conf/examples/3GB/standalone/accumulo-env.sh 7edd938 > conf/examples/512MB/native-standalone/accumulo-env.sh 749a678 > conf/examples/512MB/standalone/accumulo-env.sh 9beb059 > core/src/main/java/org/apache/accumulo/core/Constants.java 9bb3419 > core/src/main/java/org/apache/accumulo/core/conf/Property.java 424b737 > core/src/main/java/org/apache/accumulo/core/util/AddressUtil.java af9a1a6 > server/src/main/java/org/apache/accumulo/server/Accumulo.java f4da33b > server/src/main/java/org/apache/accumulo/server/monitor/LogService.java > 10ef9e4 > server/src/main/java/org/apache/accumulo/server/monitor/Monitor.java > dcb80fd > server/src/main/java/org/apache/accumulo/server/util/Info.java 7a07107 > server/src/main/java/org/apache/accumulo/server/util/Initialize.java > baa5400 > > server/src/main/java/org/apache/accumulo/server/watcher/MonitorLog4jWatcher.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/17860/diff/ > > > Testing > ------- > > Local testing so far. Configure the monitor to use a random log4j port, start > an instance. Then, test log forwarding works. Restart the monitor, check that > the tserver saw the update, and that log forwarding still works even though > the monitor is now listening on a different port for log4j. > > > Thanks, > > Josh Elser > >
