accumulo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Josh Elser" <josh.el...@gmail.com>
Subject Re: Review Request 17860: Review of changes that remove the necessity for ACCUMULO_LOG_HOST
Date Sun, 09 Feb 2014 20:15:15 GMT


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.
> 
> Josh Elser wrote:
>     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.
> 
> kturner wrote:
>     Zoolock builds on ephemeral sequential nodes.

A couple of things that address Christopher's initial thoughts: 

1) Multiple monitors are no longer a problem. Only one will be active and the rest will be
standby
2) I made ACCUMULO-2343 for the creation of an AccumuloMonitorAppender
3) The AsyncAppender which wraps the SocketAppender can *only* be configured by the DomConfigurator
and not the properties file. That's why we need the XML configuration.


- Josh


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17860/#review33975
-----------------------------------------------------------


On Feb. 9, 2014, 8:11 p.m., Josh Elser wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17860/
> -----------------------------------------------------------
> 
> (Updated Feb. 9, 2014, 8:11 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message