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.) > > Josh Elser wrote: > 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? > > kturner wrote: > These comments made me start thinking about using and ephemeral node > instead of persistent nodes in ZK. Or could possibly use zoolock and prevent > multiple monitors from running. Need to decide what behavior we want.
I think having the monitor grab a zoolock before running makes the most sense to me. I need to read up on difference of ephemeral/persistent nodes, though. I personally don't see a reason to treat the monitor any different than GC and Master in this case. - 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 > >
