You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@accumulo.apache.org by Bill Havanki <bh...@clouderagovt.com> on 2014/04/17 23:06:53 UTC
Review Request 20465: ACCUMULO-2343 - AsyncSocketAppender
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20465/
-----------------------------------------------------------
Review request for accumulo and Josh Elser.
Bugs: ACCUMULO-2343
https://issues.apache.org/jira/browse/ACCUMULO-2343
Repository: accumulo
Description
-------
AsyncSocketAppender is a Log4J AsyncAppender with its own internal SocketAppender. Configuration for either appender can be set on the AsyncSocketAppender itself. An AsyncSocketAppender can be configured using a Log4J properties file, while an ordinary AsyncAppender cannot.
Diffs
-----
core/src/main/java/org/apache/accumulo/core/util/AsyncSocketAppender.java PRE-CREATION
core/src/test/java/org/apache/accumulo/core/util/AsyncSocketAppenderTest.java PRE-CREATION
Diff: https://reviews.apache.org/r/20465/diff/
Testing
-------
Unit test created and passed. Also used main() method to send log messages to a running Log4J SimpleSocketServer instance.
Thanks,
Bill Havanki
Re: Review Request 20465: ACCUMULO-2343 - AsyncSocketAppender
Posted by Josh Elser <jo...@gmail.com>.
> On April 21, 2014, 9:08 p.m., Josh Elser wrote:
> > Did you also verify that you can run an instance using only the log4j.properties? Perhaps wrapping back around to earlier comments, have you also verified that updates to log4j.properties reload the logging backend? I'm not sure if log4j configures and watches log4j.properties by default. Also, what happens if I have both *_logger.xml files and log4j.properties files on the classpath which conflict each other (one sets some logger to WARN and the other to DEBUG) -- which one will actually be set?
>
> Bill Havanki wrote:
> I didn't perform those higher-order tests at this time because ACCUMULO-2383 covers integration testing of this appender with the rest of the system. This is a good list of things to ensure, though.
>
> From looking forward, I know that the Accumulo and MonitorLog4jWatcher classes will need to be updated to use either a Log4j DOMConfigurator or PropertyConfigurator, depending on the type of configuration file. PropertyConfigurator can set a watch on a file.
>
> The Accumulo class specifically looks for *_logger.xml files now, so we should be free to prioritize either properties or XML files in that logic. (If it were left to Log4j's default search order, then it would only look for log4j.properties.)
Gotcha, wasn't sure what all was going to get rolled into this ticket. Sounds good to punt on the integration piece for 2383. I don't think any of the integration work will require additional upstream changes (I hope?). I'll try to keep these things in mind when the big-picture changes get picked up.
- Josh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20465/#review40942
-----------------------------------------------------------
On April 18, 2014, 7:14 p.m., Bill Havanki wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20465/
> -----------------------------------------------------------
>
> (Updated April 18, 2014, 7:14 p.m.)
>
>
> Review request for accumulo and Josh Elser.
>
>
> Bugs: ACCUMULO-2343
> https://issues.apache.org/jira/browse/ACCUMULO-2343
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> AsyncSocketAppender is a Log4J AsyncAppender with its own internal SocketAppender. Configuration for either appender can be set on the AsyncSocketAppender itself. An AsyncSocketAppender can be configured using a Log4J properties file, while an ordinary AsyncAppender cannot.
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/accumulo/core/util/AsyncSocketAppender.java PRE-CREATION
> core/src/test/java/org/apache/accumulo/core/util/AsyncSocketAppenderTest.java PRE-CREATION
> test/src/test/java/org/apache/accumulo/test/functional/ConfigurableMacIT.java d9bed7f
> test/src/test/java/org/apache/accumulo/test/functional/MonitorLoggingIT.java PRE-CREATION
> test/src/test/resources/conf/generic_logger.xml PRE-CREATION
> test/src/test/resources/conf/monitor_logger.xml PRE-CREATION
>
> Diff: https://reviews.apache.org/r/20465/diff/
>
>
> Testing
> -------
>
> Unit test created and passed. Also used main() method to send log messages to a running Log4J SimpleSocketServer instance.
>
>
> Thanks,
>
> Bill Havanki
>
>
Re: Review Request 20465: ACCUMULO-2343 - AsyncSocketAppender
Posted by Bill Havanki <bh...@clouderagovt.com>.
> On April 21, 2014, 5:08 p.m., Josh Elser wrote:
> > test/src/test/java/org/apache/accumulo/test/functional/MonitorLoggingIT.java, line 48
> > <https://reviews.apache.org/r/20465/diff/2/?file=562356#file562356line48>
> >
> > Starting the Monitor before the cluster starts mind eliminate some churn. I forget if the tserver just has a watcher set to see the update or if it polls periodically.
>
> Bill Havanki wrote:
> I'll give that a go.
The test wasn't happy with trying to start the monitor before the cluster. I would get either a NoNodeException or the monitor would apparently not remain up.
> On April 21, 2014, 5:08 p.m., Josh Elser wrote:
> > test/src/test/java/org/apache/accumulo/test/functional/MonitorLoggingIT.java, line 49
> > <https://reviews.apache.org/r/20465/diff/2/?file=562356#file562356line49>
> >
> > Would be better to actually look in zookeeper for when the monitor registers itself using a Watcher than just guessing that 10s is long enough.
>
> Bill Havanki wrote:
> Quite true, I'll try that.
I added a loop checking for the monitor registration, instead of just the single attempt after 10 seconds.
> On April 21, 2014, 5:08 p.m., Josh Elser wrote:
> > test/src/test/java/org/apache/accumulo/test/functional/MonitorLoggingIT.java, line 57
> > <https://reviews.apache.org/r/20465/diff/2/?file=562356#file562356line57>
> >
> > You can easily create a monitor entry by trying to scan a table with a SKVI of "java.lang.String" or something of the sort. It would be good to ensure that you generated a message before checking.
>
> Bill Havanki wrote:
> I'll give that a shot. Definitely better than just assuming messages will appear. Thanks for all the quality feedback. :)
This worked. Good deal.
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20465/#review40942
-----------------------------------------------------------
On April 22, 2014, 10:30 a.m., Bill Havanki wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20465/
> -----------------------------------------------------------
>
> (Updated April 22, 2014, 10:30 a.m.)
>
>
> Review request for accumulo and Josh Elser.
>
>
> Bugs: ACCUMULO-2343
> https://issues.apache.org/jira/browse/ACCUMULO-2343
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> AsyncSocketAppender is a Log4J AsyncAppender with its own internal SocketAppender. Configuration for either appender can be set on the AsyncSocketAppender itself. An AsyncSocketAppender can be configured using a Log4J properties file, while an ordinary AsyncAppender cannot.
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/accumulo/core/util/AsyncSocketAppender.java PRE-CREATION
> core/src/test/java/org/apache/accumulo/core/util/AsyncSocketAppenderTest.java PRE-CREATION
> test/src/test/java/org/apache/accumulo/test/functional/ConfigurableMacIT.java d9bed7f
> test/src/test/java/org/apache/accumulo/test/functional/MonitorLoggingIT.java PRE-CREATION
> test/src/test/resources/conf/generic_logger.xml PRE-CREATION
> test/src/test/resources/conf/monitor_logger.xml PRE-CREATION
>
> Diff: https://reviews.apache.org/r/20465/diff/
>
>
> Testing
> -------
>
> Unit test created and passed. Also used main() method to send log messages to a running Log4J SimpleSocketServer instance.
>
>
> Thanks,
>
> Bill Havanki
>
>
Re: Review Request 20465: ACCUMULO-2343 - AsyncSocketAppender
Posted by Bill Havanki <bh...@clouderagovt.com>.
> On April 21, 2014, 5:08 p.m., Josh Elser wrote:
> > Did you also verify that you can run an instance using only the log4j.properties? Perhaps wrapping back around to earlier comments, have you also verified that updates to log4j.properties reload the logging backend? I'm not sure if log4j configures and watches log4j.properties by default. Also, what happens if I have both *_logger.xml files and log4j.properties files on the classpath which conflict each other (one sets some logger to WARN and the other to DEBUG) -- which one will actually be set?
I didn't perform those higher-order tests at this time because ACCUMULO-2383 covers integration testing of this appender with the rest of the system. This is a good list of things to ensure, though.
>From looking forward, I know that the Accumulo and MonitorLog4jWatcher classes will need to be updated to use either a Log4j DOMConfigurator or PropertyConfigurator, depending on the type of configuration file. PropertyConfigurator can set a watch on a file.
The Accumulo class specifically looks for *_logger.xml files now, so we should be free to prioritize either properties or XML files in that logic. (If it were left to Log4j's default search order, then it would only look for log4j.properties.)
> On April 21, 2014, 5:08 p.m., Josh Elser wrote:
> > test/src/test/java/org/apache/accumulo/test/functional/MonitorLoggingIT.java, line 48
> > <https://reviews.apache.org/r/20465/diff/2/?file=562356#file562356line48>
> >
> > Starting the Monitor before the cluster starts mind eliminate some churn. I forget if the tserver just has a watcher set to see the update or if it polls periodically.
I'll give that a go.
> On April 21, 2014, 5:08 p.m., Josh Elser wrote:
> > test/src/test/java/org/apache/accumulo/test/functional/MonitorLoggingIT.java, line 49
> > <https://reviews.apache.org/r/20465/diff/2/?file=562356#file562356line49>
> >
> > Would be better to actually look in zookeeper for when the monitor registers itself using a Watcher than just guessing that 10s is long enough.
Quite true, I'll try that.
> On April 21, 2014, 5:08 p.m., Josh Elser wrote:
> > test/src/test/java/org/apache/accumulo/test/functional/MonitorLoggingIT.java, line 57
> > <https://reviews.apache.org/r/20465/diff/2/?file=562356#file562356line57>
> >
> > You can easily create a monitor entry by trying to scan a table with a SKVI of "java.lang.String" or something of the sort. It would be good to ensure that you generated a message before checking.
I'll give that a shot. Definitely better than just assuming messages will appear. Thanks for all the quality feedback. :)
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20465/#review40942
-----------------------------------------------------------
On April 18, 2014, 3:14 p.m., Bill Havanki wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20465/
> -----------------------------------------------------------
>
> (Updated April 18, 2014, 3:14 p.m.)
>
>
> Review request for accumulo and Josh Elser.
>
>
> Bugs: ACCUMULO-2343
> https://issues.apache.org/jira/browse/ACCUMULO-2343
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> AsyncSocketAppender is a Log4J AsyncAppender with its own internal SocketAppender. Configuration for either appender can be set on the AsyncSocketAppender itself. An AsyncSocketAppender can be configured using a Log4J properties file, while an ordinary AsyncAppender cannot.
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/accumulo/core/util/AsyncSocketAppender.java PRE-CREATION
> core/src/test/java/org/apache/accumulo/core/util/AsyncSocketAppenderTest.java PRE-CREATION
> test/src/test/java/org/apache/accumulo/test/functional/ConfigurableMacIT.java d9bed7f
> test/src/test/java/org/apache/accumulo/test/functional/MonitorLoggingIT.java PRE-CREATION
> test/src/test/resources/conf/generic_logger.xml PRE-CREATION
> test/src/test/resources/conf/monitor_logger.xml PRE-CREATION
>
> Diff: https://reviews.apache.org/r/20465/diff/
>
>
> Testing
> -------
>
> Unit test created and passed. Also used main() method to send log messages to a running Log4J SimpleSocketServer instance.
>
>
> Thanks,
>
> Bill Havanki
>
>
Re: Review Request 20465: ACCUMULO-2343 - AsyncSocketAppender
Posted by Josh Elser <jo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20465/#review40942
-----------------------------------------------------------
Did you also verify that you can run an instance using only the log4j.properties? Perhaps wrapping back around to earlier comments, have you also verified that updates to log4j.properties reload the logging backend? I'm not sure if log4j configures and watches log4j.properties by default. Also, what happens if I have both *_logger.xml files and log4j.properties files on the classpath which conflict each other (one sets some logger to WARN and the other to DEBUG) -- which one will actually be set?
test/src/test/java/org/apache/accumulo/test/functional/MonitorLoggingIT.java
<https://reviews.apache.org/r/20465/#comment74265>
Starting the Monitor before the cluster starts mind eliminate some churn. I forget if the tserver just has a watcher set to see the update or if it polls periodically.
test/src/test/java/org/apache/accumulo/test/functional/MonitorLoggingIT.java
<https://reviews.apache.org/r/20465/#comment74259>
Would be better to actually look in zookeeper for when the monitor registers itself using a Watcher than just guessing that 10s is long enough.
test/src/test/java/org/apache/accumulo/test/functional/MonitorLoggingIT.java
<https://reviews.apache.org/r/20465/#comment74261>
You can easily create a monitor entry by trying to scan a table with a SKVI of "java.lang.String" or something of the sort. It would be good to ensure that you generated a message before checking.
- Josh Elser
On April 18, 2014, 7:14 p.m., Bill Havanki wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20465/
> -----------------------------------------------------------
>
> (Updated April 18, 2014, 7:14 p.m.)
>
>
> Review request for accumulo and Josh Elser.
>
>
> Bugs: ACCUMULO-2343
> https://issues.apache.org/jira/browse/ACCUMULO-2343
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> AsyncSocketAppender is a Log4J AsyncAppender with its own internal SocketAppender. Configuration for either appender can be set on the AsyncSocketAppender itself. An AsyncSocketAppender can be configured using a Log4J properties file, while an ordinary AsyncAppender cannot.
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/accumulo/core/util/AsyncSocketAppender.java PRE-CREATION
> core/src/test/java/org/apache/accumulo/core/util/AsyncSocketAppenderTest.java PRE-CREATION
> test/src/test/java/org/apache/accumulo/test/functional/ConfigurableMacIT.java d9bed7f
> test/src/test/java/org/apache/accumulo/test/functional/MonitorLoggingIT.java PRE-CREATION
> test/src/test/resources/conf/generic_logger.xml PRE-CREATION
> test/src/test/resources/conf/monitor_logger.xml PRE-CREATION
>
> Diff: https://reviews.apache.org/r/20465/diff/
>
>
> Testing
> -------
>
> Unit test created and passed. Also used main() method to send log messages to a running Log4J SimpleSocketServer instance.
>
>
> Thanks,
>
> Bill Havanki
>
>
Re: Review Request 20465: ACCUMULO-2343 - AsyncSocketAppender
Posted by Josh Elser <jo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20465/#review41023
-----------------------------------------------------------
Ship it!
Ship It!
- Josh Elser
On April 22, 2014, 2:30 p.m., Bill Havanki wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20465/
> -----------------------------------------------------------
>
> (Updated April 22, 2014, 2:30 p.m.)
>
>
> Review request for accumulo and Josh Elser.
>
>
> Bugs: ACCUMULO-2343
> https://issues.apache.org/jira/browse/ACCUMULO-2343
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> AsyncSocketAppender is a Log4J AsyncAppender with its own internal SocketAppender. Configuration for either appender can be set on the AsyncSocketAppender itself. An AsyncSocketAppender can be configured using a Log4J properties file, while an ordinary AsyncAppender cannot.
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/accumulo/core/util/AsyncSocketAppender.java PRE-CREATION
> core/src/test/java/org/apache/accumulo/core/util/AsyncSocketAppenderTest.java PRE-CREATION
> test/src/test/java/org/apache/accumulo/test/functional/ConfigurableMacIT.java d9bed7f
> test/src/test/java/org/apache/accumulo/test/functional/MonitorLoggingIT.java PRE-CREATION
> test/src/test/resources/conf/generic_logger.xml PRE-CREATION
> test/src/test/resources/conf/monitor_logger.xml PRE-CREATION
>
> Diff: https://reviews.apache.org/r/20465/diff/
>
>
> Testing
> -------
>
> Unit test created and passed. Also used main() method to send log messages to a running Log4J SimpleSocketServer instance.
>
>
> Thanks,
>
> Bill Havanki
>
>
Re: Review Request 20465: ACCUMULO-2343 - AsyncSocketAppender
Posted by Vikram Srivastava <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20465/#review41155
-----------------------------------------------------------
Ship it!
Ship It!
- Vikram Srivastava
On April 22, 2014, 2:30 p.m., Bill Havanki wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20465/
> -----------------------------------------------------------
>
> (Updated April 22, 2014, 2:30 p.m.)
>
>
> Review request for accumulo and Josh Elser.
>
>
> Bugs: ACCUMULO-2343
> https://issues.apache.org/jira/browse/ACCUMULO-2343
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> AsyncSocketAppender is a Log4J AsyncAppender with its own internal SocketAppender. Configuration for either appender can be set on the AsyncSocketAppender itself. An AsyncSocketAppender can be configured using a Log4J properties file, while an ordinary AsyncAppender cannot.
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/accumulo/core/util/AsyncSocketAppender.java PRE-CREATION
> core/src/test/java/org/apache/accumulo/core/util/AsyncSocketAppenderTest.java PRE-CREATION
> test/src/test/java/org/apache/accumulo/test/functional/ConfigurableMacIT.java d9bed7f
> test/src/test/java/org/apache/accumulo/test/functional/MonitorLoggingIT.java PRE-CREATION
> test/src/test/resources/conf/generic_logger.xml PRE-CREATION
> test/src/test/resources/conf/monitor_logger.xml PRE-CREATION
>
> Diff: https://reviews.apache.org/r/20465/diff/
>
>
> Testing
> -------
>
> Unit test created and passed. Also used main() method to send log messages to a running Log4J SimpleSocketServer instance.
>
>
> Thanks,
>
> Bill Havanki
>
>
Re: Review Request 20465: ACCUMULO-2343 - AsyncSocketAppender
Posted by Bill Havanki <bh...@clouderagovt.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20465/
-----------------------------------------------------------
(Updated April 22, 2014, 10:30 a.m.)
Review request for accumulo and Josh Elser.
Changes
-------
MonitorLoggingIT improvements based on Josh's feedback.
Bugs: ACCUMULO-2343
https://issues.apache.org/jira/browse/ACCUMULO-2343
Repository: accumulo
Description
-------
AsyncSocketAppender is a Log4J AsyncAppender with its own internal SocketAppender. Configuration for either appender can be set on the AsyncSocketAppender itself. An AsyncSocketAppender can be configured using a Log4J properties file, while an ordinary AsyncAppender cannot.
Diffs (updated)
-----
core/src/main/java/org/apache/accumulo/core/util/AsyncSocketAppender.java PRE-CREATION
core/src/test/java/org/apache/accumulo/core/util/AsyncSocketAppenderTest.java PRE-CREATION
test/src/test/java/org/apache/accumulo/test/functional/ConfigurableMacIT.java d9bed7f
test/src/test/java/org/apache/accumulo/test/functional/MonitorLoggingIT.java PRE-CREATION
test/src/test/resources/conf/generic_logger.xml PRE-CREATION
test/src/test/resources/conf/monitor_logger.xml PRE-CREATION
Diff: https://reviews.apache.org/r/20465/diff/
Testing
-------
Unit test created and passed. Also used main() method to send log messages to a running Log4J SimpleSocketServer instance.
Thanks,
Bill Havanki
Re: Review Request 20465: ACCUMULO-2343 - AsyncSocketAppender
Posted by Bill Havanki <bh...@clouderagovt.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20465/
-----------------------------------------------------------
(Updated April 18, 2014, 3:14 p.m.)
Review request for accumulo and Josh Elser.
Changes
-------
Add MonitorLoggingIT functional test, which starts a minicluster with monitor and checks that some log events show up on the monitor's log page. The AsyncSocketAppender.main() method is removed.
Bugs: ACCUMULO-2343
https://issues.apache.org/jira/browse/ACCUMULO-2343
Repository: accumulo
Description
-------
AsyncSocketAppender is a Log4J AsyncAppender with its own internal SocketAppender. Configuration for either appender can be set on the AsyncSocketAppender itself. An AsyncSocketAppender can be configured using a Log4J properties file, while an ordinary AsyncAppender cannot.
Diffs (updated)
-----
core/src/main/java/org/apache/accumulo/core/util/AsyncSocketAppender.java PRE-CREATION
core/src/test/java/org/apache/accumulo/core/util/AsyncSocketAppenderTest.java PRE-CREATION
test/src/test/java/org/apache/accumulo/test/functional/ConfigurableMacIT.java d9bed7f
test/src/test/java/org/apache/accumulo/test/functional/MonitorLoggingIT.java PRE-CREATION
test/src/test/resources/conf/generic_logger.xml PRE-CREATION
test/src/test/resources/conf/monitor_logger.xml PRE-CREATION
Diff: https://reviews.apache.org/r/20465/diff/
Testing
-------
Unit test created and passed. Also used main() method to send log messages to a running Log4J SimpleSocketServer instance.
Thanks,
Bill Havanki
Re: Review Request 20465: ACCUMULO-2343 - AsyncSocketAppender
Posted by Bill Havanki <bh...@clouderagovt.com>.
> On April 17, 2014, 5:21 p.m., Vikram Srivastava wrote:
> > We should also test that using the new Appender works fine and daemons are able to send logs to Monitor. Should we add a MacIT for that?
> >
> > Also, should we update the example log configuration files to use the new Appender?
>
> Sean Busbey wrote:
> an IT that makes use of this would be a good addition.
Updating the examples could be done either here or for ACCUMULO-2383. I'm fine either way with it. Depends on how we'd like to meter out the work.
I'll look into an IT for it. That could be a good substitute for my (shameful ;) ;) ) main method.
> On April 17, 2014, 5:21 p.m., Vikram Srivastava wrote:
> > core/src/main/java/org/apache/accumulo/core/util/AsyncSocketAppender.java, line 119
> > <https://reviews.apache.org/r/20465/diff/1/?file=561771#file561771line119>
> >
> > Is it normal to add test main methods to classes like this?
>
> Sean Busbey wrote:
> it's a bad habit, but one the Accumulo code base already has.
Indeed, I felt OK leaving it in since it's A Thing in Accumulo. Nevertheless, I have no problem removing it before commit.
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20465/#review40697
-----------------------------------------------------------
On April 17, 2014, 5:06 p.m., Bill Havanki wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20465/
> -----------------------------------------------------------
>
> (Updated April 17, 2014, 5:06 p.m.)
>
>
> Review request for accumulo and Josh Elser.
>
>
> Bugs: ACCUMULO-2343
> https://issues.apache.org/jira/browse/ACCUMULO-2343
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> AsyncSocketAppender is a Log4J AsyncAppender with its own internal SocketAppender. Configuration for either appender can be set on the AsyncSocketAppender itself. An AsyncSocketAppender can be configured using a Log4J properties file, while an ordinary AsyncAppender cannot.
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/accumulo/core/util/AsyncSocketAppender.java PRE-CREATION
> core/src/test/java/org/apache/accumulo/core/util/AsyncSocketAppenderTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/20465/diff/
>
>
> Testing
> -------
>
> Unit test created and passed. Also used main() method to send log messages to a running Log4J SimpleSocketServer instance.
>
>
> Thanks,
>
> Bill Havanki
>
>
Re: Review Request 20465: ACCUMULO-2343 - AsyncSocketAppender
Posted by Sean Busbey <se...@manvsbeard.com>.
> On April 17, 2014, 9:21 p.m., Vikram Srivastava wrote:
> > We should also test that using the new Appender works fine and daemons are able to send logs to Monitor. Should we add a MacIT for that?
> >
> > Also, should we update the example log configuration files to use the new Appender?
an IT that makes use of this would be a good addition.
> On April 17, 2014, 9:21 p.m., Vikram Srivastava wrote:
> > core/src/main/java/org/apache/accumulo/core/util/AsyncSocketAppender.java, line 119
> > <https://reviews.apache.org/r/20465/diff/1/?file=561771#file561771line119>
> >
> > Is it normal to add test main methods to classes like this?
it's a bad habit, but one the Accumulo code base already has.
- Sean
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20465/#review40697
-----------------------------------------------------------
On April 17, 2014, 9:06 p.m., Bill Havanki wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20465/
> -----------------------------------------------------------
>
> (Updated April 17, 2014, 9:06 p.m.)
>
>
> Review request for accumulo and Josh Elser.
>
>
> Bugs: ACCUMULO-2343
> https://issues.apache.org/jira/browse/ACCUMULO-2343
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> AsyncSocketAppender is a Log4J AsyncAppender with its own internal SocketAppender. Configuration for either appender can be set on the AsyncSocketAppender itself. An AsyncSocketAppender can be configured using a Log4J properties file, while an ordinary AsyncAppender cannot.
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/accumulo/core/util/AsyncSocketAppender.java PRE-CREATION
> core/src/test/java/org/apache/accumulo/core/util/AsyncSocketAppenderTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/20465/diff/
>
>
> Testing
> -------
>
> Unit test created and passed. Also used main() method to send log messages to a running Log4J SimpleSocketServer instance.
>
>
> Thanks,
>
> Bill Havanki
>
>
Re: Review Request 20465: ACCUMULO-2343 - AsyncSocketAppender
Posted by Vikram Srivastava <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20465/#review40697
-----------------------------------------------------------
We should also test that using the new Appender works fine and daemons are able to send logs to Monitor. Should we add a MacIT for that?
Also, should we update the example log configuration files to use the new Appender?
core/src/main/java/org/apache/accumulo/core/util/AsyncSocketAppender.java
<https://reviews.apache.org/r/20465/#comment73800>
Is it normal to add test main methods to classes like this?
- Vikram Srivastava
On April 17, 2014, 9:06 p.m., Bill Havanki wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20465/
> -----------------------------------------------------------
>
> (Updated April 17, 2014, 9:06 p.m.)
>
>
> Review request for accumulo and Josh Elser.
>
>
> Bugs: ACCUMULO-2343
> https://issues.apache.org/jira/browse/ACCUMULO-2343
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> AsyncSocketAppender is a Log4J AsyncAppender with its own internal SocketAppender. Configuration for either appender can be set on the AsyncSocketAppender itself. An AsyncSocketAppender can be configured using a Log4J properties file, while an ordinary AsyncAppender cannot.
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/accumulo/core/util/AsyncSocketAppender.java PRE-CREATION
> core/src/test/java/org/apache/accumulo/core/util/AsyncSocketAppenderTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/20465/diff/
>
>
> Testing
> -------
>
> Unit test created and passed. Also used main() method to send log messages to a running Log4J SimpleSocketServer instance.
>
>
> Thanks,
>
> Bill Havanki
>
>
Re: Review Request 20465: ACCUMULO-2343 - AsyncSocketAppender
Posted by Bill Havanki <bh...@clouderagovt.com>.
> On April 17, 2014, 5:34 p.m., Josh Elser wrote:
> > Are you planning to wire this up in pieces? It would be nice to be able to see how this changes the Accumulo.init(...) method which currently sets up logging for each process.
I'll take a look at init() to see how it fits in. My hope was that it would merely be a substitute merging the current "ASYNC" and "N1" appenders in the example configs. Now that I'm writing this, I think I remember seeing direct reference to the "ASYNC" name in code. I'll check.
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20465/#review40698
-----------------------------------------------------------
On April 17, 2014, 5:06 p.m., Bill Havanki wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20465/
> -----------------------------------------------------------
>
> (Updated April 17, 2014, 5:06 p.m.)
>
>
> Review request for accumulo and Josh Elser.
>
>
> Bugs: ACCUMULO-2343
> https://issues.apache.org/jira/browse/ACCUMULO-2343
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> AsyncSocketAppender is a Log4J AsyncAppender with its own internal SocketAppender. Configuration for either appender can be set on the AsyncSocketAppender itself. An AsyncSocketAppender can be configured using a Log4J properties file, while an ordinary AsyncAppender cannot.
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/accumulo/core/util/AsyncSocketAppender.java PRE-CREATION
> core/src/test/java/org/apache/accumulo/core/util/AsyncSocketAppenderTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/20465/diff/
>
>
> Testing
> -------
>
> Unit test created and passed. Also used main() method to send log messages to a running Log4J SimpleSocketServer instance.
>
>
> Thanks,
>
> Bill Havanki
>
>
Re: Review Request 20465: ACCUMULO-2343 - AsyncSocketAppender
Posted by Bill Havanki <bh...@clouderagovt.com>.
> On April 17, 2014, 5:34 p.m., Josh Elser wrote:
> > Are you planning to wire this up in pieces? It would be nice to be able to see how this changes the Accumulo.init(...) method which currently sets up logging for each process.
>
> Bill Havanki wrote:
> I'll take a look at init() to see how it fits in. My hope was that it would merely be a substitute merging the current "ASYNC" and "N1" appenders in the example configs. Now that I'm writing this, I think I remember seeing direct reference to the "ASYNC" name in code. I'll check.
>From what I can see, Accumulo.init() doesn't need to change at all if a user opts for this appender. The same logger parameters, constructed from the system properties that init() sets, are passed to a socket appender in either case: for the current setup, to a socket appender; for this one, through the new appender to its embedded socket appender.
Accumulo.init() does need to change to look for properties files as well as XML files, but I think that's for ACCUMULO-2383 or -2373.
MonitorLog4jWatcher currently specifically looks for an "ASYNC" appender to close it down in case the logging info in ZK disappears. That should work the same with the new appender, provided of course that it's also named "ASYNC". Maybe there's a way to avoid hardcoding that name.
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20465/#review40698
-----------------------------------------------------------
On April 17, 2014, 5:06 p.m., Bill Havanki wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20465/
> -----------------------------------------------------------
>
> (Updated April 17, 2014, 5:06 p.m.)
>
>
> Review request for accumulo and Josh Elser.
>
>
> Bugs: ACCUMULO-2343
> https://issues.apache.org/jira/browse/ACCUMULO-2343
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> AsyncSocketAppender is a Log4J AsyncAppender with its own internal SocketAppender. Configuration for either appender can be set on the AsyncSocketAppender itself. An AsyncSocketAppender can be configured using a Log4J properties file, while an ordinary AsyncAppender cannot.
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/accumulo/core/util/AsyncSocketAppender.java PRE-CREATION
> core/src/test/java/org/apache/accumulo/core/util/AsyncSocketAppenderTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/20465/diff/
>
>
> Testing
> -------
>
> Unit test created and passed. Also used main() method to send log messages to a running Log4J SimpleSocketServer instance.
>
>
> Thanks,
>
> Bill Havanki
>
>
Re: Review Request 20465: ACCUMULO-2343 - AsyncSocketAppender
Posted by Josh Elser <jo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20465/#review40698
-----------------------------------------------------------
Are you planning to wire this up in pieces? It would be nice to be able to see how this changes the Accumulo.init(...) method which currently sets up logging for each process.
- Josh Elser
On April 17, 2014, 9:06 p.m., Bill Havanki wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20465/
> -----------------------------------------------------------
>
> (Updated April 17, 2014, 9:06 p.m.)
>
>
> Review request for accumulo and Josh Elser.
>
>
> Bugs: ACCUMULO-2343
> https://issues.apache.org/jira/browse/ACCUMULO-2343
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> AsyncSocketAppender is a Log4J AsyncAppender with its own internal SocketAppender. Configuration for either appender can be set on the AsyncSocketAppender itself. An AsyncSocketAppender can be configured using a Log4J properties file, while an ordinary AsyncAppender cannot.
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/accumulo/core/util/AsyncSocketAppender.java PRE-CREATION
> core/src/test/java/org/apache/accumulo/core/util/AsyncSocketAppenderTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/20465/diff/
>
>
> Testing
> -------
>
> Unit test created and passed. Also used main() method to send log messages to a running Log4J SimpleSocketServer instance.
>
>
> Thanks,
>
> Bill Havanki
>
>