> 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. > > Josh Elser wrote: > 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.
ok, if its just one thread it does not matter... the use of atomic boolean and compare and set makes it appear like the code is trying to deal with concurrency > 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? > > Josh Elser wrote: > 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. another ticket sounds good > 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. > > Josh Elser wrote: > 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. there is a function called upgradeZookeeper() in Master - kturner ----------------------------------------------------------- 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 > >
