accumulo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bill Havanki" <bhava...@clouderagovt.com>
Subject Re: Review Request 15279: ACCUMULO-1556: Clarify initialization error messages for pre-initialized filesystem
Date Wed, 06 Nov 2013 20:17:58 GMT


> On Nov. 6, 2013, 2: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.

The Eclipse code formatter insisted, and I'm told we should use it.


> On Nov. 6, 2013, 2:49 p.m., Sean Busbey wrote:
> > src/server/src/main/java/org/apache/accumulo/server/util/Initialize.java, lines
71-72
> > <https://reviews.apache.org/r/15279/diff/1/?file=379597#file379597line71>
> >
> >     please don't include formatting fixes unrelated to the issue.

The Eclipse code formatter insisted, and I'm told we should use it.


> On Nov. 6, 2013, 2:49 p.m., Sean Busbey wrote:
> > src/server/src/main/java/org/apache/accumulo/server/util/Initialize.java, lines
179-181
> > <https://reviews.apache.org/r/15279/diff/1/?file=379597#file379597line179>
> >
> >     Does this buy us much beyond just letting the original IOException propagate
without wrapping?

Eh, not too much, but it states why the original IOException (upon trying to look in HDFS)
happened.


> On Nov. 6, 2013, 2:49 p.m., Sean Busbey wrote:
> > src/server/src/test/java/org/apache/accumulo/server/util/InitializeTest.java, lines
37-44
> > <https://reviews.apache.org/r/15279/diff/1/?file=379598#file379598line37>
> >
> >     Should have an @After that restores Initialize.setZooReaderWriter

Fine with me, I'll add that.


> On Nov. 6, 2013, 2: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

Will do.


> On Nov. 6, 2013, 2:49 p.m., Sean Busbey wrote:
> > src/server/src/test/java/org/apache/accumulo/server/util/InitializeTest.java, line
103
> > <https://reviews.apache.org/r/15279/diff/1/?file=379598#file379598line103>
> >
> >     Can you add a test that confirms when the underlying filesystem throws an exception
on the exists call we get an IOException back out of checkInit?

Will do.


- Bill


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15279/#review28290
-----------------------------------------------------------


On Nov. 6, 2013, 2: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, 2: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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message