----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17860/#review33975 -----------------------------------------------------------
Trivial nit: Please remove trailing whitespace throughout before applying. server/src/main/java/org/apache/accumulo/server/Accumulo.java <https://reviews.apache.org/r/17860/#comment63839> s/advetised/advertised/ Overall, I think it's an improvement, but there's some outstanding questions, like what if there are multiple monitors running... which will receive the logs? With the ACCUMULO_LOG_HOST, it was really up to the user to configure logging to point to an expected location. I never really liked the stuff we did to auto-populate that for them, but at least it was still user controllable with log configuration. Now, we're relying on registrations in ZK, and the expected behavior is a bit non-intuitive. If I start a monitor and don't realize another one is running, wouldn't it be unexpected to not get logs? Also, I think it might be better if this solution to get rid of the ACCUMULO_LOG_HOST was done by implementing a simple AccumuloMonitorAppender that extended SocketAppender, as that would be far more intuitive for users configuring log4j. (We could probably even get rid of the complex generic_logger.xml while we're at it. I think the main reason for the xml was for property interpolation after getting some information programmatically from the environment and populating system properties, but I'm not sure that's really necessary. Even if it is, I'm not sure it can't be done with the regular log4j.properties file, but maybe that specific piece, getting rid of that xml file, is another issue.) - Christopher Tubbs On Feb. 7, 2014, 5:30 p.m., Josh Elser wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/17860/ > ----------------------------------------------------------- > > (Updated Feb. 7, 2014, 5:30 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/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/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 > >
