On Feb. 7, 2014, 11:26 p.m., Josh Elser wrote: > > 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.)
Thanks for the thoughts. bq. like what if there are multiple monitors running Good point. I had thought there was a lock that the monitor got before starting, but I'm incorrect. As it stands now, if multiple monitors are running against the same instance, the one that was started last will be what gets in zk. The concern for inadvertent copies of monitors running a single instance is valid, but I can't say I've ever found myself in that situation due to the start scripts. I think being able to definitively say "this is the host:port that the SocketAppender is listening on" and outline how to use zkCli.sh to dump that information (better yet, add it to the `accumulo info` command or similar) is pretty straightforward, IMO. bq. implementing a simple AccumuloMonitorAppender that extended SocketAppender It might be cleaner, but, as it stands, I don't think your average user knows how the log-forwarding is wired up. I don't really think it changes now. When you have one monitor (excluding the case you outlined above), they still don't need to know anything -- it just works. Improvements to make configuration more straightforward seems follow-on to me, as well. Re: properties over xml log4j configuration I looked into this once, I think it's primarily just how we invoke the DomConfigurator. This is probably something we could simplify, but might be out of the scope for these changes? - Josh ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17860/#review33975 ----------------------------------------------------------- On Feb. 7, 2014, 10: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, 10: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 > >
