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