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 Sat, 08 Feb 2014 00:50:31 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.)

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
> 
>


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