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 Mon, 10 Feb 2014 23:59:24 GMT


> On Feb. 10, 2014, 11:24 p.m., kturner wrote:
> > server/src/main/java/org/apache/accumulo/server/watcher/MonitorLog4jWatcher.java,
line 120
> > <https://reviews.apache.org/r/17860/diff/2-3/?file=481201#file481201line120>
> >
> >     should this code block be synchronized?  just wondering if it should be mutually
exclusive w/ the code that enables logging.
> 
> Josh Elser wrote:
>     I don't think so because only the watcher will even trigger this block. The watcher
will only re-fire after we've left this section. The log4j reloading won't trigger this method.
> 
> kturner wrote:
>     ok, if its just one thread it does not matter... the use of atomic boolean and compare
and set makes it appear like the code is trying to deal with concurrency

Right, because there is concurrency between doChange() and updateMonitorLog4jLocation(). If
the watcher fires when the log4j watchdog fires, access on that state needs to be synchronized.

... I think I just convinced myself that there is a race condition in there now.


- Josh


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


On Feb. 10, 2014, 10:26 p.m., Josh Elser wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17860/
> -----------------------------------------------------------
> 
> (Updated Feb. 10, 2014, 10:26 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/conf/Property.java 424b737 
>   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/util/Info.java 7a07107 
>   server/src/main/java/org/apache/accumulo/server/util/Initialize.java baa5400 
>   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