You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@accumulo.apache.org by Josh Elser <jo...@gmail.com> on 2014/02/07 23:30:56 UTC
Review Request 17860: Review of changes that remove the necessity for
ACCUMULO_LOG_HOST
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17860/
-----------------------------------------------------------
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
Re: Review Request 17860: Review of changes that remove the necessity for
ACCUMULO_LOG_HOST
Posted by ke...@deenlo.com.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17860/#review34004
-----------------------------------------------------------
server/src/main/java/org/apache/accumulo/server/watcher/MonitorLog4jWatcher.java
<https://reviews.apache.org/r/17860/#comment63908>
If ZooReaderWriter.getInstance().getData(path, null) throws a NoNodeException, then catch it and turn of logging.
- kturner
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
>
>
Re: Review Request 17860: Review of changes that remove the necessity for
ACCUMULO_LOG_HOST
Posted by ke...@deenlo.com.
> On Feb. 8, 2014, 12:48 a.m., kturner wrote:
> > server/src/main/java/org/apache/accumulo/server/watcher/MonitorLog4jWatcher.java, line 95
> > <https://reviews.apache.org/r/17860/diff/1/?file=480379#file480379line95>
> >
> > If getData() throws NoNodeException (or returns null? not sure if that will happen) should probably gracefully handle this and stop trying to send log data wherever it used to be sending it.
>
> Josh Elser wrote:
> This was another direct copy/paste from what was previously there. Is there a race condition from a node going away that you have a Watcher set on? The initialization does check that the path exists before initially configuring log4j.
>
> When the monitor goes away (and "quiteMode" is turned on the logger) we just ignore the errors about not being able to talk to the remote and then the messages will eventually get dropped due to the configuration. This is the same thing as what presently happens. I have no idea if we would have any noticeable positive impact to disabling that appender (or similar) when we see that the logger goes away rather than just letting the framework roll away those messages.
I am not thinking of a race condition, just thinking of the condition where the node is there and then later goes away. If it were an ephemeral node, it would go away when the monitor processes died.
Zookeeper will call the watcher when the node goes away. Then because exist is called, the watcher will be called if it ever comes back.
- kturner
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17860/#review33988
-----------------------------------------------------------
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
>
>
Re: Review Request 17860: Review of changes that remove the necessity for
ACCUMULO_LOG_HOST
Posted by ke...@deenlo.com.
> On Feb. 8, 2014, 12:48 a.m., kturner wrote:
> > server/src/main/java/org/apache/accumulo/server/watcher/MonitorLog4jWatcher.java, line 95
> > <https://reviews.apache.org/r/17860/diff/1/?file=480379#file480379line95>
> >
> > If getData() throws NoNodeException (or returns null? not sure if that will happen) should probably gracefully handle this and stop trying to send log data wherever it used to be sending it.
>
> Josh Elser wrote:
> This was another direct copy/paste from what was previously there. Is there a race condition from a node going away that you have a Watcher set on? The initialization does check that the path exists before initially configuring log4j.
>
> When the monitor goes away (and "quiteMode" is turned on the logger) we just ignore the errors about not being able to talk to the remote and then the messages will eventually get dropped due to the configuration. This is the same thing as what presently happens. I have no idea if we would have any noticeable positive impact to disabling that appender (or similar) when we see that the logger goes away rather than just letting the framework roll away those messages.
>
> kturner wrote:
> I am not thinking of a race condition, just thinking of the condition where the node is there and then later goes away. If it were an ephemeral node, it would go away when the monitor processes died.
>
> Zookeeper will call the watcher when the node goes away. Then because exist is called, the watcher will be called if it ever comes back.
>
>
>
> Josh Elser wrote:
> So are you suggesting to just check for NodeDeleted as the type in process(WatchedEvent) before trying to pull the data out of the node? Fail-fast and (potentially) turn off logging?
I made a comment below. I made the comment while looking at the diff, I thought it would add to this thread but it did not.
- kturner
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17860/#review33988
-----------------------------------------------------------
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
>
>
Re: Review Request 17860: Review of changes that remove the necessity for
ACCUMULO_LOG_HOST
Posted by Josh Elser <jo...@gmail.com>.
> On Feb. 8, 2014, 12:48 a.m., kturner wrote:
> > server/src/main/java/org/apache/accumulo/server/watcher/MonitorLog4jWatcher.java, line 95
> > <https://reviews.apache.org/r/17860/diff/1/?file=480379#file480379line95>
> >
> > If getData() throws NoNodeException (or returns null? not sure if that will happen) should probably gracefully handle this and stop trying to send log data wherever it used to be sending it.
>
> Josh Elser wrote:
> This was another direct copy/paste from what was previously there. Is there a race condition from a node going away that you have a Watcher set on? The initialization does check that the path exists before initially configuring log4j.
>
> When the monitor goes away (and "quiteMode" is turned on the logger) we just ignore the errors about not being able to talk to the remote and then the messages will eventually get dropped due to the configuration. This is the same thing as what presently happens. I have no idea if we would have any noticeable positive impact to disabling that appender (or similar) when we see that the logger goes away rather than just letting the framework roll away those messages.
>
> kturner wrote:
> I am not thinking of a race condition, just thinking of the condition where the node is there and then later goes away. If it were an ephemeral node, it would go away when the monitor processes died.
>
> Zookeeper will call the watcher when the node goes away. Then because exist is called, the watcher will be called if it ever comes back.
>
>
So are you suggesting to just check for NodeDeleted as the type in process(WatchedEvent) before trying to pull the data out of the node? Fail-fast and (potentially) turn off logging?
- Josh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17860/#review33988
-----------------------------------------------------------
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
>
>
Re: Review Request 17860: Review of changes that remove the necessity for
ACCUMULO_LOG_HOST
Posted by Josh Elser <jo...@gmail.com>.
> On Feb. 8, 2014, 12:48 a.m., kturner wrote:
> > server/src/main/java/org/apache/accumulo/server/watcher/MonitorLog4jWatcher.java, line 95
> > <https://reviews.apache.org/r/17860/diff/1/?file=480379#file480379line95>
> >
> > If getData() throws NoNodeException (or returns null? not sure if that will happen) should probably gracefully handle this and stop trying to send log data wherever it used to be sending it.
This was another direct copy/paste from what was previously there. Is there a race condition from a node going away that you have a Watcher set on? The initialization does check that the path exists before initially configuring log4j.
When the monitor goes away (and "quiteMode" is turned on the logger) we just ignore the errors about not being able to talk to the remote and then the messages will eventually get dropped due to the configuration. This is the same thing as what presently happens. I have no idea if we would have any noticeable positive impact to disabling that appender (or similar) when we see that the logger goes away rather than just letting the framework roll away those messages.
- Josh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17860/#review33988
-----------------------------------------------------------
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
>
>
Re: Review Request 17860: Review of changes that remove the necessity for
ACCUMULO_LOG_HOST
Posted by ke...@deenlo.com.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17860/#review33988
-----------------------------------------------------------
server/src/main/java/org/apache/accumulo/server/watcher/MonitorLog4jWatcher.java
<https://reviews.apache.org/r/17860/#comment63888>
If getData() throws NoNodeException (or returns null? not sure if that will happen) should probably gracefully handle this and stop trying to send log data wherever it used to be sending it.
- kturner
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
>
>
Re: Review Request 17860: Review of changes that remove the necessity for
ACCUMULO_LOG_HOST
Posted by Mike Drob <md...@mdrob.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17860/#review33972
-----------------------------------------------------------
core/src/main/java/org/apache/accumulo/core/util/AddressUtil.java
<https://reviews.apache.org/r/17860/#comment63830>
nit: whitespace
server/src/main/java/org/apache/accumulo/server/Accumulo.java
<https://reviews.apache.org/r/17860/#comment63831>
nit: spelling "advertised"
server/src/main/java/org/apache/accumulo/server/monitor/LogService.java
<https://reviews.apache.org/r/17860/#comment63832>
nit: whitespace
server/src/main/java/org/apache/accumulo/server/monitor/LogService.java
<https://reviews.apache.org/r/17860/#comment63833>
nit: whitespace
server/src/main/java/org/apache/accumulo/server/monitor/LogService.java
<https://reviews.apache.org/r/17860/#comment63834>
nit: whitespace
server/src/main/java/org/apache/accumulo/server/monitor/Monitor.java
<https://reviews.apache.org/r/17860/#comment63835>
nit: whitespace
server/src/main/java/org/apache/accumulo/server/monitor/Monitor.java
<https://reviews.apache.org/r/17860/#comment63836>
nit: whitespace
server/src/main/java/org/apache/accumulo/server/watcher/MonitorLog4jWatcher.java
<https://reviews.apache.org/r/17860/#comment63842>
Do we still want to run if there was an error?
- Mike Drob
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
>
>
Re: Review Request 17860: Review of changes that remove the necessity for
ACCUMULO_LOG_HOST
Posted by Josh Elser <jo...@gmail.com>.
> 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
>
>
Re: Review Request 17860: Review of changes that remove the necessity for
ACCUMULO_LOG_HOST
Posted by Josh Elser <jo...@gmail.com>.
> On Feb. 10, 2014, 11:24 p.m., kturner wrote:
> > server/src/main/java/org/apache/accumulo/server/monitor/Monitor.java, line 595
> > <https://reviews.apache.org/r/17860/diff/2-3/?file=481200#file481200line595>
> >
> > Should this code be shared w/ master instead of duplicating?
>
> Josh Elser wrote:
> Probably. I noticed that there's also duplication with this same sort of watcher with the garbage collector. I'll make a follow on to consolidate these watchers across all processes.
>
> kturner wrote:
> another ticket sounds good
ACCUMULO-2348
> On Feb. 10, 2014, 11:24 p.m., kturner wrote:
> > server/src/main/java/org/apache/accumulo/server/monitor/Monitor.java, line 554
> > <https://reviews.apache.org/r/17860/diff/2-3/?file=481200#file481200line554>
> >
> > Have you manually tested upgrade?
> >
> > It may be better to put this w/ the zookeeper upgrade code in Master. Would make it easier when someones working on 1.7 upgrade to be aware of it and possibly remove it.
>
> Josh Elser wrote:
> Yes. I tested it against a 1.5.1-SNAPSHOT install I had locally that did not yet have these changes to ZooKeeper and it worked just fine.
>
> I can move it over to the Master code. I'll look at doing that to make sure it makes sense.
>
> kturner wrote:
> there is a function called upgradeZookeeper() in Master
I don't think it makes sense to move the code into the Master. As it stands now, we can trigger the zookeeper check when the monitor starts up. If we move it to the monitor, we'd have to wait for the Master to come up and finish the upgrade, and then start. I think it's simpler to just leave it in the Monitor.
- 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
>
>
Re: Review Request 17860: Review of changes that remove the necessity for
ACCUMULO_LOG_HOST
Posted by ke...@deenlo.com.
> On Feb. 10, 2014, 11:24 p.m., kturner wrote:
> > server/src/main/java/org/apache/accumulo/server/monitor/Monitor.java, line 554
> > <https://reviews.apache.org/r/17860/diff/2-3/?file=481200#file481200line554>
> >
> > Have you manually tested upgrade?
> >
> > It may be better to put this w/ the zookeeper upgrade code in Master. Would make it easier when someones working on 1.7 upgrade to be aware of it and possibly remove it.
>
> Josh Elser wrote:
> Yes. I tested it against a 1.5.1-SNAPSHOT install I had locally that did not yet have these changes to ZooKeeper and it worked just fine.
>
> I can move it over to the Master code. I'll look at doing that to make sure it makes sense.
>
> kturner wrote:
> there is a function called upgradeZookeeper() in Master
>
> Josh Elser wrote:
> I don't think it makes sense to move the code into the Master. As it stands now, we can trigger the zookeeper check when the monitor starts up. If we move it to the monitor, we'd have to wait for the Master to come up and finish the upgrade, and then start. I think it's simpler to just leave it in the Monitor.
I agree w/ what you said. Maybe a better solution is to put all of the upgrade code in one package or class. Master can call code here and monitor can call method here to upgrade itself. This would put all of the upgrade related code in one place making it easier for future upgrade related changes. Or maybe put a comment in the master upgrade code that mentions there is also upgrade code in the monitor, so at least there is a pointer to it. M
- kturner
-----------------------------------------------------------
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
>
>
Re: Review Request 17860: Review of changes that remove the necessity for
ACCUMULO_LOG_HOST
Posted by ke...@deenlo.com.
> 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.
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
> On Feb. 10, 2014, 11:24 p.m., kturner wrote:
> > server/src/main/java/org/apache/accumulo/server/monitor/Monitor.java, line 595
> > <https://reviews.apache.org/r/17860/diff/2-3/?file=481200#file481200line595>
> >
> > Should this code be shared w/ master instead of duplicating?
>
> Josh Elser wrote:
> Probably. I noticed that there's also duplication with this same sort of watcher with the garbage collector. I'll make a follow on to consolidate these watchers across all processes.
another ticket sounds good
> On Feb. 10, 2014, 11:24 p.m., kturner wrote:
> > server/src/main/java/org/apache/accumulo/server/monitor/Monitor.java, line 554
> > <https://reviews.apache.org/r/17860/diff/2-3/?file=481200#file481200line554>
> >
> > Have you manually tested upgrade?
> >
> > It may be better to put this w/ the zookeeper upgrade code in Master. Would make it easier when someones working on 1.7 upgrade to be aware of it and possibly remove it.
>
> Josh Elser wrote:
> Yes. I tested it against a 1.5.1-SNAPSHOT install I had locally that did not yet have these changes to ZooKeeper and it worked just fine.
>
> I can move it over to the Master code. I'll look at doing that to make sure it makes sense.
there is a function called upgradeZookeeper() in Master
- kturner
-----------------------------------------------------------
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
>
>
Re: Review Request 17860: Review of changes that remove the necessity for
ACCUMULO_LOG_HOST
Posted by Josh Elser <jo...@gmail.com>.
> 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.
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.
> On Feb. 10, 2014, 11:24 p.m., kturner wrote:
> > server/src/main/java/org/apache/accumulo/server/monitor/Monitor.java, line 595
> > <https://reviews.apache.org/r/17860/diff/2-3/?file=481200#file481200line595>
> >
> > Should this code be shared w/ master instead of duplicating?
Probably. I noticed that there's also duplication with this same sort of watcher with the garbage collector. I'll make a follow on to consolidate these watchers across all processes.
> On Feb. 10, 2014, 11:24 p.m., kturner wrote:
> > server/src/main/java/org/apache/accumulo/server/monitor/Monitor.java, line 554
> > <https://reviews.apache.org/r/17860/diff/2-3/?file=481200#file481200line554>
> >
> > Have you manually tested upgrade?
> >
> > It may be better to put this w/ the zookeeper upgrade code in Master. Would make it easier when someones working on 1.7 upgrade to be aware of it and possibly remove it.
Yes. I tested it against a 1.5.1-SNAPSHOT install I had locally that did not yet have these changes to ZooKeeper and it worked just fine.
I can move it over to the Master code. I'll look at doing that to make sure it makes sense.
> On Feb. 10, 2014, 11:24 p.m., kturner wrote:
> > server/src/main/java/org/apache/accumulo/server/watcher/MonitorLog4jWatcher.java, line 116
> > <https://reviews.apache.org/r/17860/diff/3/?file=482201#file482201line116>
> >
> > stylistic comment, feel free to ignore.
> >
> >
> > Could catch NoNodeException (before catching KeeperException) and put code in if(NONODE){} in catch block
Ahh, good call. I didn't peek into the hierarchy of KeeperException. I like your suggestion.
- 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
>
>
Re: Review Request 17860: Review of changes that remove the necessity for
ACCUMULO_LOG_HOST
Posted by Josh Elser <jo...@gmail.com>.
> On Feb. 10, 2014, 11:24 p.m., kturner wrote:
> > server/src/main/java/org/apache/accumulo/server/monitor/Monitor.java, line 554
> > <https://reviews.apache.org/r/17860/diff/2-3/?file=481200#file481200line554>
> >
> > Have you manually tested upgrade?
> >
> > It may be better to put this w/ the zookeeper upgrade code in Master. Would make it easier when someones working on 1.7 upgrade to be aware of it and possibly remove it.
>
> Josh Elser wrote:
> Yes. I tested it against a 1.5.1-SNAPSHOT install I had locally that did not yet have these changes to ZooKeeper and it worked just fine.
>
> I can move it over to the Master code. I'll look at doing that to make sure it makes sense.
>
> kturner wrote:
> there is a function called upgradeZookeeper() in Master
>
> Josh Elser wrote:
> I don't think it makes sense to move the code into the Master. As it stands now, we can trigger the zookeeper check when the monitor starts up. If we move it to the monitor, we'd have to wait for the Master to come up and finish the upgrade, and then start. I think it's simpler to just leave it in the Monitor.
>
> kturner wrote:
> I agree w/ what you said. Maybe a better solution is to put all of the upgrade code in one package or class. Master can call code here and monitor can call method here to upgrade itself. This would put all of the upgrade related code in one place making it easier for future upgrade related changes. Or maybe put a comment in the master upgrade code that mentions there is also upgrade code in the monitor, so at least there is a pointer to it. M
SGTM. I'll also make a ticket for 1.7 about consolidating upgrade code in one place (server-base sounds like an intelligent location off the cuff).
- 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
>
>
Re: Review Request 17860: Review of changes that remove the necessity for
ACCUMULO_LOG_HOST
Posted by ke...@deenlo.com.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17860/#review34124
-----------------------------------------------------------
server/src/main/java/org/apache/accumulo/server/monitor/Monitor.java
<https://reviews.apache.org/r/17860/#comment64101>
Have you manually tested upgrade?
It may be better to put this w/ the zookeeper upgrade code in Master. Would make it easier when someones working on 1.7 upgrade to be aware of it and possibly remove it.
server/src/main/java/org/apache/accumulo/server/monitor/Monitor.java
<https://reviews.apache.org/r/17860/#comment64099>
Should this code be shared w/ master instead of duplicating?
server/src/main/java/org/apache/accumulo/server/watcher/MonitorLog4jWatcher.java
<https://reviews.apache.org/r/17860/#comment64110>
should this code block be synchronized? just wondering if it should be mutually exclusive w/ the code that enables logging.
server/src/main/java/org/apache/accumulo/server/watcher/MonitorLog4jWatcher.java
<https://reviews.apache.org/r/17860/#comment64116>
stylistic comment, feel free to ignore.
Could catch NoNodeException (before catching KeeperException) and put code in if(NONODE){} in catch block
- kturner
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
>
>
Re: Review Request 17860: Review of changes that remove the necessity for
ACCUMULO_LOG_HOST
Posted by Josh Elser <jo...@gmail.com>.
-----------------------------------------------------------
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.
Changes
-------
Apparently I botched the last patch update.
Fixes whitespace nits. Disable SocketAppender when monitor goes away, re-enable on configuration file update or ZK update. Re-work the monitor info in ZK and force the Monitor to grab a lock before starting (prevents multiple active monitors with only one getting remote logging).
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 (updated)
-----
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
Re: Review Request 17860: Review of changes that remove the necessity for
ACCUMULO_LOG_HOST
Posted by Josh Elser <jo...@gmail.com>.
-----------------------------------------------------------
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.
Changes
-------
Fixes whitespace nits. Disable SocketAppender when monitor goes away, re-enable on configuration file update or ZK update. Re-work the monitor info in ZK and force the Monitor to grab a lock before starting (prevents multiple active monitors with only one getting remote logging).
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 (updated)
-----
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
Re: Review Request 17860: Review of changes that remove the necessity for
ACCUMULO_LOG_HOST
Posted by ke...@deenlo.com.
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?
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.
- kturner
-----------------------------------------------------------
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
>
>
Re: Review Request 17860: Review of changes that remove the necessity for
ACCUMULO_LOG_HOST
Posted by Josh Elser <jo...@gmail.com>.
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
>
>
Re: Review Request 17860: Review of changes that remove the necessity for
ACCUMULO_LOG_HOST
Posted by ke...@deenlo.com.
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.
Zoolock builds on ephemeral sequential nodes.
- kturner
-----------------------------------------------------------
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
>
>
Re: Review Request 17860: Review of changes that remove the necessity for
ACCUMULO_LOG_HOST
Posted by Josh Elser <jo...@gmail.com>.
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
>
>
Re: Review Request 17860: Review of changes that remove the necessity for
ACCUMULO_LOG_HOST
Posted by Josh Elser <jo...@gmail.com>.
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
>
>
Re: Review Request 17860: Review of changes that remove the necessity for
ACCUMULO_LOG_HOST
Posted by Christopher Tubbs <ct...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17860/#review33975
-----------------------------------------------------------
Trivial nit: Please remove trailing whitespace throughout before applying.
server/src/main/java/org/apache/accumulo/server/Accumulo.java
<https://reviews.apache.org/r/17860/#comment63839>
s/advetised/advertised/
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.)
- Christopher Tubbs
On Feb. 7, 2014, 5: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, 5: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
>
>
Re: Review Request 17860: Review of changes that remove the necessity for
ACCUMULO_LOG_HOST
Posted by Josh Elser <jo...@gmail.com>.
> On Feb. 7, 2014, 10:40 p.m., Josh Elser wrote:
> > server/src/main/java/org/apache/accumulo/server/Accumulo.java, line 173
> > <https://reviews.apache.org/r/17860/diff/1/?file=480376#file480376line173>
> >
> > Perhaps it would be better to increase this timeout to something like 30s instead of 5s? I'm a little concerned about having a thundering herd of 100s of servers bash zookeeper with reads. I'm not really sure if that's something to worry about though.
This timeout isn't for zookeeper, it's for watching the local generic_logger.xml file for updates. The ZK concern still exists, but it's not something that's controllable with a timeout/interval.
- Josh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17860/#review33974
-----------------------------------------------------------
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
>
>
Re: Review Request 17860: Review of changes that remove the necessity for
ACCUMULO_LOG_HOST
Posted by Josh Elser <jo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17860/#review33974
-----------------------------------------------------------
server/src/main/java/org/apache/accumulo/server/Accumulo.java
<https://reviews.apache.org/r/17860/#comment63838>
Perhaps it would be better to increase this timeout to something like 30s instead of 5s? I'm a little concerned about having a thundering herd of 100s of servers bash zookeeper with reads. I'm not really sure if that's something to worry about though.
- Josh Elser
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
>
>
Re: Review Request 17860: Review of changes that remove the necessity for
ACCUMULO_LOG_HOST
Posted by Josh Elser <jo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17860/#review33976
-----------------------------------------------------------
Noticed that I can fix up some formatting too. I'll update that while I'm at it.
- Josh Elser
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
>
>