> 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. > > kturner wrote: > 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
Right, because there is concurrency between doChange() and updateMonitorLog4jLocation(). If the watcher fires when the log4j watchdog fires, access on that state needs to be synchronized. ... I think I just convinced myself that there is a race condition in there now. - 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 > >
