You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@accumulo.apache.org by Christopher Tubbs <ct...@apache.org> on 2014/05/01 22:06:36 UTC
Re: Review Request 20613: ACCUMULO-2383 - support for Log4j properties
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20613/#review41940
-----------------------------------------------------------
server/base/src/main/java/org/apache/accumulo/server/Accumulo.java
<https://reviews.apache.org/r/20613/#comment75599>
I think it'd be better to prefer properties file over xml.
- Christopher Tubbs
On April 23, 2014, 4:37 p.m., Bill Havanki wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20613/
> -----------------------------------------------------------
>
> (Updated April 23, 2014, 4:37 p.m.)
>
>
> Review request for accumulo, Josh Elser and Vikram Srivastava.
>
>
> Bugs: ACCUMULO-2383
> https://issues.apache.org/jira/browse/ACCUMULO-2383
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> Accumulo now looks for either XML or properties files for the Log4J configuration. Its MonitorLog4jWatcher can now load either properties or XML.
>
> This code depends on the v3 patch of ACCUMULO-2343; follow the "Depends On" link to jump to that review.
>
> Note on MonitorLog4jWatcher: Its constructor was calling setDelay() on its superclass, but setDelay() is non-final. I took this opportunity to fix that.
>
>
> Diffs
> -----
>
> conf/templates/generic_logger.properties PRE-CREATION
> conf/templates/monitor_logger.properties PRE-CREATION
> server/base/src/main/java/org/apache/accumulo/server/Accumulo.java 4e1eb35
> server/base/src/main/java/org/apache/accumulo/server/watcher/MonitorLog4jWatcher.java ac3426e
> server/base/src/test/java/org/apache/accumulo/server/AccumuloTest.java 9366163
> server/base/src/test/java/org/apache/accumulo/server/watcher/MonitorLog4jWatcherTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/20613/diff/
>
>
> Testing
> -------
>
> - New unit tests pass, such as they are.
> - Ran servers with XML and with properties configurations on single-node cluster. Observed reloading for properties files (didn't re-test XML). Checked that all logs were being generated. Observed log messages sent from master and tserver to monitor, displayed on web.
> - Ran short (50k-hop) randomwalk tests: Security, Concurrent, MultiTable.
>
>
> Thanks,
>
> Bill Havanki
>
>
Re: Review Request 20613: ACCUMULO-2383 - support for Log4j properties
Posted by Bill Havanki <bh...@clouderagovt.com>.
> On May 1, 2014, 4:06 p.m., Christopher Tubbs wrote:
> > server/base/src/main/java/org/apache/accumulo/server/Accumulo.java, lines 128-131
> > <https://reviews.apache.org/r/20613/diff/2/?file=566102#file566102line128>
> >
> > I think it'd be better to prefer properties file over xml.
My reasons to prefer XML: 1) our examples already use XML; 2) XML configurations are more flexible and powerful than properties.
Using XML configurations is mildly encouraged now. Still, Log4J only looks for properties in its default init procedure (AFAICT), so perhaps the ordering here would be surprising to a user?
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20613/#review41940
-----------------------------------------------------------
On April 23, 2014, 4:37 p.m., Bill Havanki wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20613/
> -----------------------------------------------------------
>
> (Updated April 23, 2014, 4:37 p.m.)
>
>
> Review request for accumulo, Josh Elser and Vikram Srivastava.
>
>
> Bugs: ACCUMULO-2383
> https://issues.apache.org/jira/browse/ACCUMULO-2383
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> Accumulo now looks for either XML or properties files for the Log4J configuration. Its MonitorLog4jWatcher can now load either properties or XML.
>
> This code depends on the v3 patch of ACCUMULO-2343; follow the "Depends On" link to jump to that review.
>
> Note on MonitorLog4jWatcher: Its constructor was calling setDelay() on its superclass, but setDelay() is non-final. I took this opportunity to fix that.
>
>
> Diffs
> -----
>
> conf/templates/generic_logger.properties PRE-CREATION
> conf/templates/monitor_logger.properties PRE-CREATION
> server/base/src/main/java/org/apache/accumulo/server/Accumulo.java 4e1eb35
> server/base/src/main/java/org/apache/accumulo/server/watcher/MonitorLog4jWatcher.java ac3426e
> server/base/src/test/java/org/apache/accumulo/server/AccumuloTest.java 9366163
> server/base/src/test/java/org/apache/accumulo/server/watcher/MonitorLog4jWatcherTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/20613/diff/
>
>
> Testing
> -------
>
> - New unit tests pass, such as they are.
> - Ran servers with XML and with properties configurations on single-node cluster. Observed reloading for properties files (didn't re-test XML). Checked that all logs were being generated. Observed log messages sent from master and tserver to monitor, displayed on web.
> - Ran short (50k-hop) randomwalk tests: Security, Concurrent, MultiTable.
>
>
> Thanks,
>
> Bill Havanki
>
>
Re: Review Request 20613: ACCUMULO-2383 - support for Log4j properties
Posted by Christopher Tubbs <ct...@apache.org>.
> On May 1, 2014, 4:06 p.m., Christopher Tubbs wrote:
> > server/base/src/main/java/org/apache/accumulo/server/Accumulo.java, lines 128-131
> > <https://reviews.apache.org/r/20613/diff/2/?file=566102#file566102line128>
> >
> > I think it'd be better to prefer properties file over xml.
>
> Bill Havanki wrote:
> My reasons to prefer XML: 1) our examples already use XML; 2) XML configurations are more flexible and powerful than properties.
>
> Using XML configurations is mildly encouraged now. Still, Log4J only looks for properties in its default init procedure (AFAICT), so perhaps the ordering here would be surprising to a user?
>
> Christopher Tubbs wrote:
> Standard log4j behavior is to load the properties file first, and fall back on xml if log4j properties doesn't exist, I think.
> For consistency and intuitiveness, I think that's reasonable behavior to mimic. It won't affect any existing user configurations, where the properties file doesn't exist.
>
> Re: 1) I'd like to see the xml examples replaced with properties after 1.6.x, but that's a follow-on ticket. Future examples should be simpler than the most powerful use case.
> Re: 2) I don't think many people exercise the additional flexibility and power the xml files offer. If they do, it sounds like that's not going to be the primary case. I'd rather promote the properties files over xml, personally. So long as it doesn't break the existing examples, I don't see an issue with switching the order.
>
> Josh Elser wrote:
> IMO, changing the priority should be done later. I see no reason to not apply this content as it stands and we can evaluate the implications of switching from xml to properties as a separate task (including documentation updates, release note additions, and configuration updates, etc)
Sure, I have no problem changing the order later, when we do updated examples.
- Christopher
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20613/#review41940
-----------------------------------------------------------
On April 23, 2014, 4:37 p.m., Bill Havanki wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20613/
> -----------------------------------------------------------
>
> (Updated April 23, 2014, 4:37 p.m.)
>
>
> Review request for accumulo, Josh Elser and Vikram Srivastava.
>
>
> Bugs: ACCUMULO-2383
> https://issues.apache.org/jira/browse/ACCUMULO-2383
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> Accumulo now looks for either XML or properties files for the Log4J configuration. Its MonitorLog4jWatcher can now load either properties or XML.
>
> This code depends on the v3 patch of ACCUMULO-2343; follow the "Depends On" link to jump to that review.
>
> Note on MonitorLog4jWatcher: Its constructor was calling setDelay() on its superclass, but setDelay() is non-final. I took this opportunity to fix that.
>
>
> Diffs
> -----
>
> conf/templates/generic_logger.properties PRE-CREATION
> conf/templates/monitor_logger.properties PRE-CREATION
> server/base/src/main/java/org/apache/accumulo/server/Accumulo.java 4e1eb35
> server/base/src/main/java/org/apache/accumulo/server/watcher/MonitorLog4jWatcher.java ac3426e
> server/base/src/test/java/org/apache/accumulo/server/AccumuloTest.java 9366163
> server/base/src/test/java/org/apache/accumulo/server/watcher/MonitorLog4jWatcherTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/20613/diff/
>
>
> Testing
> -------
>
> - New unit tests pass, such as they are.
> - Ran servers with XML and with properties configurations on single-node cluster. Observed reloading for properties files (didn't re-test XML). Checked that all logs were being generated. Observed log messages sent from master and tserver to monitor, displayed on web.
> - Ran short (50k-hop) randomwalk tests: Security, Concurrent, MultiTable.
>
>
> Thanks,
>
> Bill Havanki
>
>
Re: Review Request 20613: ACCUMULO-2383 - support for Log4j properties
Posted by Christopher Tubbs <ct...@apache.org>.
> On May 1, 2014, 4:06 p.m., Christopher Tubbs wrote:
> > server/base/src/main/java/org/apache/accumulo/server/Accumulo.java, lines 128-131
> > <https://reviews.apache.org/r/20613/diff/2/?file=566102#file566102line128>
> >
> > I think it'd be better to prefer properties file over xml.
>
> Bill Havanki wrote:
> My reasons to prefer XML: 1) our examples already use XML; 2) XML configurations are more flexible and powerful than properties.
>
> Using XML configurations is mildly encouraged now. Still, Log4J only looks for properties in its default init procedure (AFAICT), so perhaps the ordering here would be surprising to a user?
Standard log4j behavior is to load the properties file first, and fall back on xml if log4j properties doesn't exist, I think.
For consistency and intuitiveness, I think that's reasonable behavior to mimic. It won't affect any existing user configurations, where the properties file doesn't exist.
Re: 1) I'd like to see the xml examples replaced with properties after 1.6.x, but that's a follow-on ticket. Future examples should be simpler than the most powerful use case.
Re: 2) I don't think many people exercise the additional flexibility and power the xml files offer. If they do, it sounds like that's not going to be the primary case. I'd rather promote the properties files over xml, personally. So long as it doesn't break the existing examples, I don't see an issue with switching the order.
- Christopher
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20613/#review41940
-----------------------------------------------------------
On April 23, 2014, 4:37 p.m., Bill Havanki wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20613/
> -----------------------------------------------------------
>
> (Updated April 23, 2014, 4:37 p.m.)
>
>
> Review request for accumulo, Josh Elser and Vikram Srivastava.
>
>
> Bugs: ACCUMULO-2383
> https://issues.apache.org/jira/browse/ACCUMULO-2383
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> Accumulo now looks for either XML or properties files for the Log4J configuration. Its MonitorLog4jWatcher can now load either properties or XML.
>
> This code depends on the v3 patch of ACCUMULO-2343; follow the "Depends On" link to jump to that review.
>
> Note on MonitorLog4jWatcher: Its constructor was calling setDelay() on its superclass, but setDelay() is non-final. I took this opportunity to fix that.
>
>
> Diffs
> -----
>
> conf/templates/generic_logger.properties PRE-CREATION
> conf/templates/monitor_logger.properties PRE-CREATION
> server/base/src/main/java/org/apache/accumulo/server/Accumulo.java 4e1eb35
> server/base/src/main/java/org/apache/accumulo/server/watcher/MonitorLog4jWatcher.java ac3426e
> server/base/src/test/java/org/apache/accumulo/server/AccumuloTest.java 9366163
> server/base/src/test/java/org/apache/accumulo/server/watcher/MonitorLog4jWatcherTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/20613/diff/
>
>
> Testing
> -------
>
> - New unit tests pass, such as they are.
> - Ran servers with XML and with properties configurations on single-node cluster. Observed reloading for properties files (didn't re-test XML). Checked that all logs were being generated. Observed log messages sent from master and tserver to monitor, displayed on web.
> - Ran short (50k-hop) randomwalk tests: Security, Concurrent, MultiTable.
>
>
> Thanks,
>
> Bill Havanki
>
>
Re: Review Request 20613: ACCUMULO-2383 - support for Log4j properties
Posted by Josh Elser <jo...@gmail.com>.
> On May 1, 2014, 8:06 p.m., Christopher Tubbs wrote:
> > server/base/src/main/java/org/apache/accumulo/server/Accumulo.java, lines 128-131
> > <https://reviews.apache.org/r/20613/diff/2/?file=566102#file566102line128>
> >
> > I think it'd be better to prefer properties file over xml.
>
> Bill Havanki wrote:
> My reasons to prefer XML: 1) our examples already use XML; 2) XML configurations are more flexible and powerful than properties.
>
> Using XML configurations is mildly encouraged now. Still, Log4J only looks for properties in its default init procedure (AFAICT), so perhaps the ordering here would be surprising to a user?
>
> Christopher Tubbs wrote:
> Standard log4j behavior is to load the properties file first, and fall back on xml if log4j properties doesn't exist, I think.
> For consistency and intuitiveness, I think that's reasonable behavior to mimic. It won't affect any existing user configurations, where the properties file doesn't exist.
>
> Re: 1) I'd like to see the xml examples replaced with properties after 1.6.x, but that's a follow-on ticket. Future examples should be simpler than the most powerful use case.
> Re: 2) I don't think many people exercise the additional flexibility and power the xml files offer. If they do, it sounds like that's not going to be the primary case. I'd rather promote the properties files over xml, personally. So long as it doesn't break the existing examples, I don't see an issue with switching the order.
IMO, changing the priority should be done later. I see no reason to not apply this content as it stands and we can evaluate the implications of switching from xml to properties as a separate task (including documentation updates, release note additions, and configuration updates, etc)
- Josh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20613/#review41940
-----------------------------------------------------------
On April 23, 2014, 8:37 p.m., Bill Havanki wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20613/
> -----------------------------------------------------------
>
> (Updated April 23, 2014, 8:37 p.m.)
>
>
> Review request for accumulo, Josh Elser and Vikram Srivastava.
>
>
> Bugs: ACCUMULO-2383
> https://issues.apache.org/jira/browse/ACCUMULO-2383
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> Accumulo now looks for either XML or properties files for the Log4J configuration. Its MonitorLog4jWatcher can now load either properties or XML.
>
> This code depends on the v3 patch of ACCUMULO-2343; follow the "Depends On" link to jump to that review.
>
> Note on MonitorLog4jWatcher: Its constructor was calling setDelay() on its superclass, but setDelay() is non-final. I took this opportunity to fix that.
>
>
> Diffs
> -----
>
> conf/templates/generic_logger.properties PRE-CREATION
> conf/templates/monitor_logger.properties PRE-CREATION
> server/base/src/main/java/org/apache/accumulo/server/Accumulo.java 4e1eb35
> server/base/src/main/java/org/apache/accumulo/server/watcher/MonitorLog4jWatcher.java ac3426e
> server/base/src/test/java/org/apache/accumulo/server/AccumuloTest.java 9366163
> server/base/src/test/java/org/apache/accumulo/server/watcher/MonitorLog4jWatcherTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/20613/diff/
>
>
> Testing
> -------
>
> - New unit tests pass, such as they are.
> - Ran servers with XML and with properties configurations on single-node cluster. Observed reloading for properties files (didn't re-test XML). Checked that all logs were being generated. Observed log messages sent from master and tserver to monitor, displayed on web.
> - Ran short (50k-hop) randomwalk tests: Security, Concurrent, MultiTable.
>
>
> Thanks,
>
> Bill Havanki
>
>