> On Nov. 6, 2013, 7:49 p.m., Sean Busbey wrote: > > src/server/src/main/java/org/apache/accumulo/server/util/Initialize.java, > > lines 23-25 > > <https://reviews.apache.org/r/15279/diff/1/?file=379597#file379597line23> > > > > please don't include formatting fixes unrelatd to the change. > > Bill Havanki wrote: > The Eclipse code formatter insisted, and I'm told we should use it. > > Sean Busbey wrote: > Break into another issue/patch then please.
For code you introduce, sure. I wouldn't worry yourself with it here. There is an eclipse option to only format lines you modify. > On Nov. 6, 2013, 7:49 p.m., Sean Busbey wrote: > > src/server/src/test/java/org/apache/accumulo/server/util/InitializeTest.java, > > line 31 > > <https://reviews.apache.org/r/15279/diff/1/?file=379598#file379598line31> > > > > Include a note that this test class is not threadsafe. > > > > The default poms and build scripts don't run parallel unit tests. But I > > know the build speed is something we care about, so we'll get there > > eventually. Best to have a warning for future maintainers. How much detail > > to include is up to you. > > > > It won't work with JUnit 4.7+'s parallel, since they're in the same JVM: > > > > > > http://maven.apache.org/surefire/maven-surefire-plugin/examples/junit.html#Running_tests_in_parallel > > > > It will work with Surefire's fork and reuseForks=false, since that's a > > JVM per class > > > > > > http://maven.apache.org/surefire/maven-surefire-plugin/examples/fork-options-and-parallel-execution.html#Forked_Test_Execution > > Bill Havanki wrote: > Will do. > > Bill Havanki wrote: > Another thought: we could start to introduce the JCIP annotations to > indicate thread safety (or lack of it) in a better way. I'll float the idea > to the dev list. > > Sean Busbey wrote: > Sure. Don't let the wider refactor that would be stop you from finishing > this issue though. :) > > I think starting to use JCIP annotations should be its own ticket. 1.4.5 and 1.5.1 are bugfix releases, and I'm really only concerns about fixing the ticket on hand. I'm really iffy to even qualify it as a something that should be fixed in those releases as this really is an improvement, not a bug. But it seems useful and lightweight, so fixing the ticket at hand is fine. But introducing a whole bunch of other things in 1.4.x with it I'm going to have to vote against. - John ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/15279/#review28290 ----------------------------------------------------------- On Nov. 6, 2013, 7:02 p.m., Bill Havanki wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/15279/ > ----------------------------------------------------------- > > (Updated Nov. 6, 2013, 7:02 p.m.) > > > Review request for accumulo. > > > Bugs: ACCUMULO-1556 > https://issues.apache.org/jira/browse/ACCUMULO-1556 > > > Repository: accumulo > > > Description > ------- > > The Initialize class now generates clearer error messages if an initialized > instance is discovered. The messages vary depending on whether > instance.dfs.uri is used. > > Note that to facilitate unit testing, the verification logic in > Initialize.doInit() was refactored into a checkInit() method. > > > Diffs > ----- > > pom.xml 9ed2fdf1c7a1f8831667b27bfaa307fbe2467fe8 > src/server/pom.xml 6421bc69cda5116f9716d30adc3d510baa3bb7d6 > src/server/src/main/java/org/apache/accumulo/server/util/Initialize.java > 51576fcc8ff8ffcd65a78bfeaaaf51036360a6bc > > src/server/src/test/java/org/apache/accumulo/server/util/InitializeTest.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/15279/diff/ > > > Testing > ------- > > Ran initialization with patch changes on 1.4.3 cluster under CDH 4.3. Tested > successful initialization and correct emission of error messages when > instance.dfs.uri was used and was not used. Also, implemented unit tests for > altered code. > > > Thanks, > > Bill Havanki > >
