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 2013/11/06 20:02:03 UTC
Review Request 15279: ACCUMULO-1556: Clarify initialization error messages
for pre-initialized filesystem
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15279/
-----------------------------------------------------------
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
Re: Review Request 15279: ACCUMULO-1556: Clarify initialization error
messages for pre-initialized filesystem
Posted by John Vines <vi...@apache.org>.
> On Nov. 6, 2013, 8 p.m., John Vines wrote:
> > Ship It!
Grr, misclicked and I don't seem to be able to delete it...
- John
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15279/#review28296
-----------------------------------------------------------
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
>
>
Re: Review Request 15279: ACCUMULO-1556: Clarify initialization error
messages for pre-initialized filesystem
Posted by Sean Busbey <se...@manvsbeard.com>.
> On Nov. 6, 2013, 8 p.m., John Vines wrote:
> > Ship It!
>
> John Vines wrote:
> Grr, misclicked and I don't seem to be able to delete it...
>
> John Vines wrote:
> Seriously hating review board, ate the rest of my comment.
>
> My issues are Busbey's issues. I don't think the thread safe ones are that high priority, but that's a nice to have.
we could ask INFRA for a Gerrit instance.
- Sean
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15279/#review28296
-----------------------------------------------------------
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
>
>
Re: Review Request 15279: ACCUMULO-1556: Clarify initialization error
messages for pre-initialized filesystem
Posted by John Vines <vi...@apache.org>.
> On Nov. 6, 2013, 8 p.m., John Vines wrote:
> > Ship It!
>
> John Vines wrote:
> Grr, misclicked and I don't seem to be able to delete it...
Seriously hating review board, ate the rest of my comment.
My issues are Busbey's issues. I don't think the thread safe ones are that high priority, but that's a nice to have.
- John
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15279/#review28296
-----------------------------------------------------------
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
>
>
Re: Review Request 15279: ACCUMULO-1556: Clarify initialization error
messages for pre-initialized filesystem
Posted by John Vines <vi...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15279/#review28296
-----------------------------------------------------------
Ship it!
Ship It!
- John Vines
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
>
>
Re: Review Request 15279: ACCUMULO-1556: Clarify initialization error
messages for pre-initialized filesystem
Posted by Sean Busbey <se...@manvsbeard.com>.
> 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.
>
> John Vines wrote:
> 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.
That's a major reason I want JCIP annotations in their own ticket. I presume Bill was talking about using them primarily in the current major version, but all of this is discussion unrelated to the task at hand.
The current issue just needs a comment block so that 1.4, 1.5, and 1.6 have a suitable warning.
- Sean
-----------------------------------------------------------
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
>
>
Re: Review Request 15279: ACCUMULO-1556: Clarify initialization error
messages for pre-initialized filesystem
Posted by Bill Havanki <bh...@clouderagovt.com>.
> 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
>
> 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.
>
> John Vines wrote:
> 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.
>
> Sean Busbey wrote:
> That's a major reason I want JCIP annotations in their own ticket. I presume Bill was talking about using them primarily in the current major version, but all of this is discussion unrelated to the task at hand.
>
> The current issue just needs a comment block so that 1.4, 1.5, and 1.6 have a suitable warning.
My intention is not to try introducing JCIP with this ticket. I concur with Sean, it's its own ticket.
- 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
>
>
Re: Review Request 15279: ACCUMULO-1556: Clarify initialization error
messages for pre-initialized filesystem
Posted by John Vines <vi...@apache.org>.
> 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
>
>
Re: Review Request 15279: ACCUMULO-1556: Clarify initialization error
messages for pre-initialized filesystem
Posted by Bill Havanki <bh...@clouderagovt.com>.
> 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.
>
> 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.
>
> John Vines wrote:
> For code you introduce, sure. I wouldn't worry yourself with it here. There is an eclipse option to only format lines you modify.
Sounds good. I'll revert these two bits.
- 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
>
>
Re: Review Request 15279: ACCUMULO-1556: Clarify initialization error
messages for pre-initialized filesystem
Posted by Bill Havanki <bh...@clouderagovt.com>.
> 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
>
>
Re: Review Request 15279: ACCUMULO-1556: Clarify initialization error
messages for pre-initialized filesystem
Posted by Sean Busbey <se...@manvsbeard.com>.
> 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.
Break into another issue/patch then please.
> On Nov. 6, 2013, 7: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.
>
> Bill Havanki wrote:
> The Eclipse code formatter insisted, and I'm told we should use it.
Break into another issue/patch then please.
> On Nov. 6, 2013, 7: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?
>
> Bill Havanki wrote:
> Eh, not too much, but it states why the original IOException (upon trying to look in HDFS) happened.
Fair enough.
> 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.
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.
- Sean
-----------------------------------------------------------
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
>
>
Re: Review Request 15279: ACCUMULO-1556: Clarify initialization error
messages for pre-initialized filesystem
Posted by Bill Havanki <bh...@clouderagovt.com>.
> 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
>
> Bill Havanki wrote:
> Will do.
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.
- 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
>
>
Re: Review Request 15279: ACCUMULO-1556: Clarify initialization error
messages for pre-initialized filesystem
Posted by Sean Busbey <se...@manvsbeard.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15279/#review28290
-----------------------------------------------------------
src/server/src/main/java/org/apache/accumulo/server/util/Initialize.java
<https://reviews.apache.org/r/15279/#comment55071>
please don't include formatting fixes unrelatd to the change.
src/server/src/main/java/org/apache/accumulo/server/util/Initialize.java
<https://reviews.apache.org/r/15279/#comment55072>
please don't include formatting fixes unrelated to the issue.
src/server/src/main/java/org/apache/accumulo/server/util/Initialize.java
<https://reviews.apache.org/r/15279/#comment55074>
Does this buy us much beyond just letting the original IOException propagate without wrapping?
src/server/src/test/java/org/apache/accumulo/server/util/InitializeTest.java
<https://reviews.apache.org/r/15279/#comment55087>
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
src/server/src/test/java/org/apache/accumulo/server/util/InitializeTest.java
<https://reviews.apache.org/r/15279/#comment55079>
Should have an @After that restores Initialize.setZooReaderWriter
src/server/src/test/java/org/apache/accumulo/server/util/InitializeTest.java
<https://reviews.apache.org/r/15279/#comment55092>
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?
- Sean Busbey
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
>
>
Re: Review Request 15279: ACCUMULO-1556: Clarify initialization error
messages for pre-initialized filesystem
Posted by Sean Busbey <se...@manvsbeard.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15279/#review28312
-----------------------------------------------------------
Ship it!
src/server/src/main/java/org/apache/accumulo/server/util/Initialize.java
<https://reviews.apache.org/r/15279/#comment55121>
nit: don't add whitespace errors.
- Sean Busbey
On Nov. 6, 2013, 8:47 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, 8:47 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
>
>
Re: Review Request 15279: ACCUMULO-1556: Clarify initialization error
messages for pre-initialized filesystem
Posted by Bill Havanki <bh...@clouderagovt.com>.
> On Nov. 6, 2013, 4:12 p.m., kturner wrote:
> > src/server/src/main/java/org/apache/accumulo/server/util/Initialize.java, line 168
> > <https://reviews.apache.org/r/15279/diff/2/?file=379903#file379903line168>
> >
> > It would also be useful to include the value of Property.INSTANCE_DFS_DIR in this error message.
Will do.
> On Nov. 6, 2013, 4:12 p.m., kturner wrote:
> > src/server/src/main/java/org/apache/accumulo/server/util/Initialize.java, line 131
> > <https://reviews.apache.org/r/15279/diff/2/?file=379903#file379903line131>
> >
> > I think this code for determining the fsUri is redundant. In main() the filesystem is obtained by calling FileUtil.getFileSystem() which uses INSTANCE_DFS_URI. Therefore maybe just call fs.getUri().
You are correct. I think it still needs to peek at the property to determine whether the filesystem URI was retrieved from the property or the default. Is that right?
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15279/#review28316
-----------------------------------------------------------
On Nov. 6, 2013, 3:47 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, 3:47 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
>
>
Re: Review Request 15279: ACCUMULO-1556: Clarify initialization error
messages for pre-initialized filesystem
Posted by ke...@deenlo.com.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15279/#review28316
-----------------------------------------------------------
src/server/src/main/java/org/apache/accumulo/server/util/Initialize.java
<https://reviews.apache.org/r/15279/#comment55124>
I think this code for determining the fsUri is redundant. In main() the filesystem is obtained by calling FileUtil.getFileSystem() which uses INSTANCE_DFS_URI. Therefore maybe just call fs.getUri().
src/server/src/main/java/org/apache/accumulo/server/util/Initialize.java
<https://reviews.apache.org/r/15279/#comment55125>
It would also be useful to include the value of Property.INSTANCE_DFS_DIR in this error message.
- kturner
On Nov. 6, 2013, 8:47 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, 8:47 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
>
>
Re: Review Request 15279: ACCUMULO-1556: Clarify initialization error
messages for pre-initialized filesystem
Posted by Josh Elser <jo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15279/#review28415
-----------------------------------------------------------
Ship it!
Ship It!
- Josh Elser
On Nov. 7, 2013, 7:09 p.m., Bill Havanki wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15279/
> -----------------------------------------------------------
>
> (Updated Nov. 7, 2013, 7:09 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
>
>
Re: Review Request 15279: ACCUMULO-1556: Clarify initialization error
messages for pre-initialized filesystem
Posted by Bill Havanki <bh...@clouderagovt.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15279/
-----------------------------------------------------------
(Updated Nov. 7, 2013, 2:09 p.m.)
Review request for accumulo.
Changes
-------
- Location now shown as filesystem URI + directory.
- Added suggestion to change instance.dfs.dir.
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 (updated)
-----
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
Re: Review Request 15279: ACCUMULO-1556: Clarify initialization error
messages for pre-initialized filesystem
Posted by Josh Elser <jo...@gmail.com>.
On 11/7/13, 2:22 PM, Michael Berman wrote:
>
>
> > I made the above comment yesterday, but never pressed the publish
> button on RB
>
> I do this ALL THE TIME :)
>
>
>
> Speaking of RB quirks...do other people frequently see server errors
> after hitting publish? I get a proxy error on the PUT more often than
> not, but so far it seems like it always actually gets in.
Yeah I just started seeing this today.
Re: Review Request 15279: ACCUMULO-1556: Clarify initialization error
messages for pre-initialized filesystem
Posted by Michael Berman <mb...@sqrrl.com>.
>
> > I made the above comment yesterday, but never pressed the publish button
> on RB
>
> I do this ALL THE TIME :)
Speaking of RB quirks...do other people frequently see server errors after
hitting publish? I get a proxy error on the PUT more often than not, but
so far it seems like it always actually gets in.
Re: Review Request 15279: ACCUMULO-1556: Clarify initialization error
messages for pre-initialized filesystem
Posted by Bill Havanki <bh...@clouderagovt.com>.
> On Nov. 6, 2013, 5:08 p.m., kturner wrote:
> > src/server/src/main/java/org/apache/accumulo/server/util/Initialize.java, line 162
> > <https://reviews.apache.org/r/15279/diff/3/?file=380105#file380105line162>
> >
> > Accumulo uses a dir within a filesystem. It does not init an entire filesystem.
> >
> > The message should indicate that fsUri+sconf.get(Property.INSTANCE_DFS_DIR) was initialized.
>
> Bill Havanki wrote:
> Nice catch. Question: Should the messages also suggest that instance.dfs.dir could be changed to use a different directory within the chosen filesystem?
>
> Josh Elser wrote:
> That would be an appropriate suggestion.
>
> kturner wrote:
> Yeah that seems helpful.
>
> I made the above comment yesterday, but never pressed the publish button on RB
> I made the above comment yesterday, but never pressed the publish button on RB
I do this ALL THE TIME :)
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15279/#review28322
-----------------------------------------------------------
On Nov. 7, 2013, 2:09 p.m., Bill Havanki wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15279/
> -----------------------------------------------------------
>
> (Updated Nov. 7, 2013, 2:09 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
>
>
Re: Review Request 15279: ACCUMULO-1556: Clarify initialization error
messages for pre-initialized filesystem
Posted by Josh Elser <jo...@gmail.com>.
> On Nov. 6, 2013, 10:08 p.m., kturner wrote:
> > src/server/src/main/java/org/apache/accumulo/server/util/Initialize.java, line 162
> > <https://reviews.apache.org/r/15279/diff/3/?file=380105#file380105line162>
> >
> > Accumulo uses a dir within a filesystem. It does not init an entire filesystem.
> >
> > The message should indicate that fsUri+sconf.get(Property.INSTANCE_DFS_DIR) was initialized.
>
> Bill Havanki wrote:
> Nice catch. Question: Should the messages also suggest that instance.dfs.dir could be changed to use a different directory within the chosen filesystem?
That would be an appropriate suggestion.
- Josh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15279/#review28322
-----------------------------------------------------------
On Nov. 6, 2013, 9:59 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, 9:59 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
>
>
Re: Review Request 15279: ACCUMULO-1556: Clarify initialization error
messages for pre-initialized filesystem
Posted by Bill Havanki <bh...@clouderagovt.com>.
> On Nov. 6, 2013, 5:08 p.m., kturner wrote:
> > src/server/src/main/java/org/apache/accumulo/server/util/Initialize.java, line 162
> > <https://reviews.apache.org/r/15279/diff/3/?file=380105#file380105line162>
> >
> > Accumulo uses a dir within a filesystem. It does not init an entire filesystem.
> >
> > The message should indicate that fsUri+sconf.get(Property.INSTANCE_DFS_DIR) was initialized.
Nice catch. Question: Should the messages also suggest that instance.dfs.dir could be changed to use a different directory within the chosen filesystem?
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15279/#review28322
-----------------------------------------------------------
On Nov. 6, 2013, 4:59 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, 4:59 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
>
>
Re: Review Request 15279: ACCUMULO-1556: Clarify initialization error
messages for pre-initialized filesystem
Posted by ke...@deenlo.com.
> On Nov. 6, 2013, 10:08 p.m., kturner wrote:
> > src/server/src/main/java/org/apache/accumulo/server/util/Initialize.java, line 162
> > <https://reviews.apache.org/r/15279/diff/3/?file=380105#file380105line162>
> >
> > Accumulo uses a dir within a filesystem. It does not init an entire filesystem.
> >
> > The message should indicate that fsUri+sconf.get(Property.INSTANCE_DFS_DIR) was initialized.
>
> Bill Havanki wrote:
> Nice catch. Question: Should the messages also suggest that instance.dfs.dir could be changed to use a different directory within the chosen filesystem?
>
> Josh Elser wrote:
> That would be an appropriate suggestion.
Yeah that seems helpful.
I made the above comment yesterday, but never pressed the publish button on RB
- kturner
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15279/#review28322
-----------------------------------------------------------
On Nov. 6, 2013, 9:59 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, 9:59 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
>
>
Re: Review Request 15279: ACCUMULO-1556: Clarify initialization error
messages for pre-initialized filesystem
Posted by ke...@deenlo.com.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15279/#review28322
-----------------------------------------------------------
src/server/src/main/java/org/apache/accumulo/server/util/Initialize.java
<https://reviews.apache.org/r/15279/#comment55142>
Accumulo uses a dir within a filesystem. It does not init an entire filesystem.
The message should indicate that fsUri+sconf.get(Property.INSTANCE_DFS_DIR) was initialized.
- kturner
On Nov. 6, 2013, 9:59 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, 9:59 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
>
>
Re: Review Request 15279: ACCUMULO-1556: Clarify initialization error
messages for pre-initialized filesystem
Posted by Bill Havanki <bh...@clouderagovt.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15279/
-----------------------------------------------------------
(Updated Nov. 6, 2013, 4:59 p.m.)
Review request for accumulo.
Changes
-------
- Error messages for pre-initialized filesystem now include current
value of instance.dfs.uri (in pipes because the default is "")
- Retrieve filesystem URI value from filesystem object itself instead
of repeating work of FileUtil.getFileSystem()
- Minor whitespace fix
- Correcting formatting for InitializeTest from last two diffs
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 (updated)
-----
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
Re: Review Request 15279: ACCUMULO-1556: Clarify initialization error
messages for pre-initialized filesystem
Posted by Bill Havanki <bh...@clouderagovt.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15279/
-----------------------------------------------------------
(Updated Nov. 6, 2013, 3:47 p.m.)
Review request for accumulo.
Changes
-------
Updates to address issues in first diff:
- Revert reformatting in top matter of Initialize.java
- Marked InitializeTest as not thread-safe
- Added @After to restore ZK reader/writer in InitializeTest
- Added test of IOException accessing HDFS to InitializeTest
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 (updated)
-----
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