You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@accumulo.apache.org by ke...@deenlo.com on 2014/09/02 19:45:31 UTC
Review Request 25260: Throw exception on metadata constraint
violation, instead of retrying
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25260/
-----------------------------------------------------------
Review request for accumulo.
Bugs: ACCUMULO-3096
https://issues.apache.org/jira/browse/ACCUMULO-3096
Repository: accumulo
Description
-------
Throw exception on metadata constraint violation, instead of retrying
Diffs
-----
core/src/main/java/org/apache/accumulo/core/client/impl/Writer.java d6762e7
server/base/src/main/java/org/apache/accumulo/server/util/MetadataTableUtil.java 463ca57
test/src/test/java/org/apache/accumulo/test/Accumulo3096IT.java PRE-CREATION
Diff: https://reviews.apache.org/r/25260/diff/
Testing
-------
Ran mvn package w/o incident . Currently running mvn verify.
Thanks,
kturner
Re: Review Request 25260: Throw exception on metadata constraint
violation, instead of retrying
Posted by Josh Elser <jo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25260/#review52179
-----------------------------------------------------------
Ship it!
Ship It!
- Josh Elser
On Sept. 3, 2014, 3:48 p.m., kturner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25260/
> -----------------------------------------------------------
>
> (Updated Sept. 3, 2014, 3:48 p.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-3096
> https://issues.apache.org/jira/browse/ACCUMULO-3096
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> Throw exception on metadata constraint violation, instead of retrying
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/accumulo/core/client/impl/Writer.java d6762e7
> server/base/src/main/java/org/apache/accumulo/server/util/MetadataTableUtil.java 463ca57
> test/src/test/java/org/apache/accumulo/test/MetaConstraintRetryIT.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/25260/diff/
>
>
> Testing
> -------
>
> Ran mvn package w/o incident . Currently running mvn verify.
>
>
> Thanks,
>
> kturner
>
>
Re: Review Request 25260: Throw exception on metadata constraint
violation, instead of retrying
Posted by ke...@deenlo.com.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25260/
-----------------------------------------------------------
(Updated Sept. 3, 2014, 3:48 p.m.)
Review request for accumulo.
Changes
-------
missed josh's comment about a comment
Bugs: ACCUMULO-3096
https://issues.apache.org/jira/browse/ACCUMULO-3096
Repository: accumulo
Description
-------
Throw exception on metadata constraint violation, instead of retrying
Diffs (updated)
-----
core/src/main/java/org/apache/accumulo/core/client/impl/Writer.java d6762e7
server/base/src/main/java/org/apache/accumulo/server/util/MetadataTableUtil.java 463ca57
test/src/test/java/org/apache/accumulo/test/MetaConstraintRetryIT.java PRE-CREATION
Diff: https://reviews.apache.org/r/25260/diff/
Testing
-------
Ran mvn package w/o incident . Currently running mvn verify.
Thanks,
kturner
Re: Review Request 25260: Throw exception on metadata constraint
violation, instead of retrying
Posted by ke...@deenlo.com.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25260/
-----------------------------------------------------------
(Updated Sept. 3, 2014, 3:36 p.m.)
Review request for accumulo.
Changes
-------
possibly cleaned up exception handling in test :)
Bugs: ACCUMULO-3096
https://issues.apache.org/jira/browse/ACCUMULO-3096
Repository: accumulo
Description
-------
Throw exception on metadata constraint violation, instead of retrying
Diffs (updated)
-----
core/src/main/java/org/apache/accumulo/core/client/impl/Writer.java d6762e7
server/base/src/main/java/org/apache/accumulo/server/util/MetadataTableUtil.java 463ca57
test/src/test/java/org/apache/accumulo/test/MetaConstraintRetryIT.java PRE-CREATION
Diff: https://reviews.apache.org/r/25260/diff/
Testing
-------
Ran mvn package w/o incident . Currently running mvn verify.
Thanks,
kturner
Re: Review Request 25260: Throw exception on metadata constraint
violation, instead of retrying
Posted by ke...@deenlo.com.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25260/
-----------------------------------------------------------
(Updated Sept. 3, 2014, 3:17 p.m.)
Review request for accumulo.
Changes
-------
renamed test
Bugs: ACCUMULO-3096
https://issues.apache.org/jira/browse/ACCUMULO-3096
Repository: accumulo
Description
-------
Throw exception on metadata constraint violation, instead of retrying
Diffs (updated)
-----
core/src/main/java/org/apache/accumulo/core/client/impl/Writer.java d6762e7
server/base/src/main/java/org/apache/accumulo/server/util/MetadataTableUtil.java 463ca57
test/src/test/java/org/apache/accumulo/test/MetaConstraintRetryIT.java PRE-CREATION
Diff: https://reviews.apache.org/r/25260/diff/
Testing
-------
Ran mvn package w/o incident . Currently running mvn verify.
Thanks,
kturner
Re: Review Request 25260: Throw exception on metadata constraint
violation, instead of retrying
Posted by ke...@deenlo.com.
> On Sept. 2, 2014, 10:32 p.m., Josh Elser wrote:
> > test/src/test/java/org/apache/accumulo/test/Accumulo3096IT.java, line 53
> > <https://reviews.apache.org/r/25260/diff/1/?file=674070#file674070line53>
> >
> > Would be better as an Assert.equals() call with a message as to why we expect the CVE as the cause.
If its not the expected exception, I like throwing it. Then we see what the stack trace for the unexpected exception which is useful for debugging.
- kturner
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25260/#review52094
-----------------------------------------------------------
On Sept. 3, 2014, 3:17 p.m., kturner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25260/
> -----------------------------------------------------------
>
> (Updated Sept. 3, 2014, 3:17 p.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-3096
> https://issues.apache.org/jira/browse/ACCUMULO-3096
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> Throw exception on metadata constraint violation, instead of retrying
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/accumulo/core/client/impl/Writer.java d6762e7
> server/base/src/main/java/org/apache/accumulo/server/util/MetadataTableUtil.java 463ca57
> test/src/test/java/org/apache/accumulo/test/MetaConstraintRetryIT.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/25260/diff/
>
>
> Testing
> -------
>
> Ran mvn package w/o incident . Currently running mvn verify.
>
>
> Thanks,
>
> kturner
>
>
Re: Review Request 25260: Throw exception on metadata constraint
violation, instead of retrying
Posted by Christopher Tubbs <ct...@apache.org>.
> On Sept. 2, 2014, 6:32 p.m., Josh Elser wrote:
> > test/src/test/java/org/apache/accumulo/test/Accumulo3096IT.java, line 34
> > <https://reviews.apache.org/r/25260/diff/1/?file=674070#file674070line34>
> >
> > It'd be much nicer to have a test name that is more meaningful than a ticket. Having the ticket referenced in a comment is useful if I want to find out more, but at first glance it doesn't tell me anything about what it's testing.
>
> Mike Drob wrote:
> Why is it even useful as a comment? It should already be in the commit message and 'git blame' will tell you who and where it came from.
>
> Josh Elser wrote:
> That's valid -- a bit more personal preference, I suppose. I prefer to have information in the code rather than have to look at the history to find when it was introduced, especially when the most recent change isn't from the ticket that originally introduced it (when blame would be wrong).
One can't assume that a person inspecting the released code has access to the history or the JIRA. Those are nice supplemental resources, but understanding what is being tested, at a basic level, should not depend on them. A meaningful test name gives some scope to the issue in log output without a lookup, regardless of access to history or JIRA, so I think the name change suggestion is a good idea. Further, Josh's suggestion to bump the JIRA number to a comment I think also is a good idea, because it provides additional linkage to the JIRA in the absence of the git history.
- Christopher
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25260/#review52094
-----------------------------------------------------------
On Sept. 2, 2014, 1:45 p.m., kturner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25260/
> -----------------------------------------------------------
>
> (Updated Sept. 2, 2014, 1:45 p.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-3096
> https://issues.apache.org/jira/browse/ACCUMULO-3096
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> Throw exception on metadata constraint violation, instead of retrying
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/accumulo/core/client/impl/Writer.java d6762e7
> server/base/src/main/java/org/apache/accumulo/server/util/MetadataTableUtil.java 463ca57
> test/src/test/java/org/apache/accumulo/test/Accumulo3096IT.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/25260/diff/
>
>
> Testing
> -------
>
> Ran mvn package w/o incident . Currently running mvn verify.
>
>
> Thanks,
>
> kturner
>
>
Re: Review Request 25260: Throw exception on metadata constraint
violation, instead of retrying
Posted by ke...@deenlo.com.
> On Sept. 2, 2014, 10:32 p.m., Josh Elser wrote:
> > test/src/test/java/org/apache/accumulo/test/Accumulo3096IT.java, line 34
> > <https://reviews.apache.org/r/25260/diff/1/?file=674070#file674070line34>
> >
> > It'd be much nicer to have a test name that is more meaningful than a ticket. Having the ticket referenced in a comment is useful if I want to find out more, but at first glance it doesn't tell me anything about what it's testing.
>
> Mike Drob wrote:
> Why is it even useful as a comment? It should already be in the commit message and 'git blame' will tell you who and where it came from.
>
> Josh Elser wrote:
> That's valid -- a bit more personal preference, I suppose. I prefer to have information in the code rather than have to look at the history to find when it was introduced, especially when the most recent change isn't from the ticket that originally introduced it (when blame would be wrong).
>
> Christopher Tubbs wrote:
> One can't assume that a person inspecting the released code has access to the history or the JIRA. Those are nice supplemental resources, but understanding what is being tested, at a basic level, should not depend on them. A meaningful test name gives some scope to the issue in log output without a lookup, regardless of access to history or JIRA, so I think the name change suggestion is a good idea. Further, Josh's suggestion to bump the JIRA number to a comment I think also is a good idea, because it provides additional linkage to the JIRA in the absence of the git history.
I started thinking how this would play out over time. If we had 50 or 60 test named like this, that would require a script to make sense of it. I will rename the test to ConstratintViolationRetryIT. I am not sure if RB will handle renames when diffing patches, so I am thinking of making a 2nd patch w/ just the rename and a 3rd patch w/ the other changes. Doing this to ensure it remains easy to diff the patches.
> On Sept. 2, 2014, 10:32 p.m., Josh Elser wrote:
> > server/base/src/main/java/org/apache/accumulo/server/util/MetadataTableUtil.java, line 149
> > <https://reviews.apache.org/r/25260/diff/1/?file=674069#file674069line149>
> >
> > A comment about why we throw a RTE for ConstraintViolationException but none of the others would be nice (in other words, reference the reason these changes are being done).
>
> Mike Drob wrote:
> HBase has a DoNotRetryIOException, maybe something like that would be interesting to consider.
That does seems like an interesting concept. Wouldn't this be a bigger change and maybe its own issue? Doesn't seem like we would want to introduce something like that in this one place.
- kturner
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25260/#review52094
-----------------------------------------------------------
On Sept. 2, 2014, 5:45 p.m., kturner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25260/
> -----------------------------------------------------------
>
> (Updated Sept. 2, 2014, 5:45 p.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-3096
> https://issues.apache.org/jira/browse/ACCUMULO-3096
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> Throw exception on metadata constraint violation, instead of retrying
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/accumulo/core/client/impl/Writer.java d6762e7
> server/base/src/main/java/org/apache/accumulo/server/util/MetadataTableUtil.java 463ca57
> test/src/test/java/org/apache/accumulo/test/Accumulo3096IT.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/25260/diff/
>
>
> Testing
> -------
>
> Ran mvn package w/o incident . Currently running mvn verify.
>
>
> Thanks,
>
> kturner
>
>
Re: Review Request 25260: Throw exception on metadata constraint
violation, instead of retrying
Posted by Christopher Tubbs <ct...@apache.org>.
> On Sept. 2, 2014, 6:32 p.m., Josh Elser wrote:
> > test/src/test/java/org/apache/accumulo/test/Accumulo3096IT.java, line 34
> > <https://reviews.apache.org/r/25260/diff/1/?file=674070#file674070line34>
> >
> > It'd be much nicer to have a test name that is more meaningful than a ticket. Having the ticket referenced in a comment is useful if I want to find out more, but at first glance it doesn't tell me anything about what it's testing.
>
> Mike Drob wrote:
> Why is it even useful as a comment? It should already be in the commit message and 'git blame' will tell you who and where it came from.
>
> Josh Elser wrote:
> That's valid -- a bit more personal preference, I suppose. I prefer to have information in the code rather than have to look at the history to find when it was introduced, especially when the most recent change isn't from the ticket that originally introduced it (when blame would be wrong).
>
> Christopher Tubbs wrote:
> One can't assume that a person inspecting the released code has access to the history or the JIRA. Those are nice supplemental resources, but understanding what is being tested, at a basic level, should not depend on them. A meaningful test name gives some scope to the issue in log output without a lookup, regardless of access to history or JIRA, so I think the name change suggestion is a good idea. Further, Josh's suggestion to bump the JIRA number to a comment I think also is a good idea, because it provides additional linkage to the JIRA in the absence of the git history.
>
> kturner wrote:
> I started thinking how this would play out over time. If we had 50 or 60 test named like this, that would require a script to make sense of it. I will rename the test to ConstratintViolationRetryIT. I am not sure if RB will handle renames when diffing patches, so I am thinking of making a 2nd patch w/ just the rename and a 3rd patch w/ the other changes. Doing this to ensure it remains easy to diff the patches.
>
> kturner wrote:
> RB did not seem to pickup the rename when I diffed patches 1 and 2
Probably could have saved the rename until after the review. :)
- Christopher
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25260/#review52094
-----------------------------------------------------------
On Sept. 3, 2014, 11:48 a.m., kturner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25260/
> -----------------------------------------------------------
>
> (Updated Sept. 3, 2014, 11:48 a.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-3096
> https://issues.apache.org/jira/browse/ACCUMULO-3096
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> Throw exception on metadata constraint violation, instead of retrying
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/accumulo/core/client/impl/Writer.java d6762e7
> server/base/src/main/java/org/apache/accumulo/server/util/MetadataTableUtil.java 463ca57
> test/src/test/java/org/apache/accumulo/test/MetaConstraintRetryIT.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/25260/diff/
>
>
> Testing
> -------
>
> Ran mvn package w/o incident . Currently running mvn verify.
>
>
> Thanks,
>
> kturner
>
>
Re: Review Request 25260: Throw exception on metadata constraint
violation, instead of retrying
Posted by ke...@deenlo.com.
> On Sept. 2, 2014, 10:32 p.m., Josh Elser wrote:
> > test/src/test/java/org/apache/accumulo/test/Accumulo3096IT.java, line 51
> > <https://reviews.apache.org/r/25260/diff/1/?file=674070#file674070line51>
> >
> > Message for the failure would be nice.
fail() is gone in the latest change
> On Sept. 2, 2014, 10:32 p.m., Josh Elser wrote:
> > test/src/test/java/org/apache/accumulo/test/Accumulo3096IT.java, line 34
> > <https://reviews.apache.org/r/25260/diff/1/?file=674070#file674070line34>
> >
> > It'd be much nicer to have a test name that is more meaningful than a ticket. Having the ticket referenced in a comment is useful if I want to find out more, but at first glance it doesn't tell me anything about what it's testing.
>
> Mike Drob wrote:
> Why is it even useful as a comment? It should already be in the commit message and 'git blame' will tell you who and where it came from.
>
> Josh Elser wrote:
> That's valid -- a bit more personal preference, I suppose. I prefer to have information in the code rather than have to look at the history to find when it was introduced, especially when the most recent change isn't from the ticket that originally introduced it (when blame would be wrong).
>
> Christopher Tubbs wrote:
> One can't assume that a person inspecting the released code has access to the history or the JIRA. Those are nice supplemental resources, but understanding what is being tested, at a basic level, should not depend on them. A meaningful test name gives some scope to the issue in log output without a lookup, regardless of access to history or JIRA, so I think the name change suggestion is a good idea. Further, Josh's suggestion to bump the JIRA number to a comment I think also is a good idea, because it provides additional linkage to the JIRA in the absence of the git history.
>
> kturner wrote:
> I started thinking how this would play out over time. If we had 50 or 60 test named like this, that would require a script to make sense of it. I will rename the test to ConstratintViolationRetryIT. I am not sure if RB will handle renames when diffing patches, so I am thinking of making a 2nd patch w/ just the rename and a 3rd patch w/ the other changes. Doing this to ensure it remains easy to diff the patches.
RB did not seem to pickup the rename when I diffed patches 1 and 2
- kturner
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25260/#review52094
-----------------------------------------------------------
On Sept. 3, 2014, 3:36 p.m., kturner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25260/
> -----------------------------------------------------------
>
> (Updated Sept. 3, 2014, 3:36 p.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-3096
> https://issues.apache.org/jira/browse/ACCUMULO-3096
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> Throw exception on metadata constraint violation, instead of retrying
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/accumulo/core/client/impl/Writer.java d6762e7
> server/base/src/main/java/org/apache/accumulo/server/util/MetadataTableUtil.java 463ca57
> test/src/test/java/org/apache/accumulo/test/MetaConstraintRetryIT.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/25260/diff/
>
>
> Testing
> -------
>
> Ran mvn package w/o incident . Currently running mvn verify.
>
>
> Thanks,
>
> kturner
>
>
Re: Review Request 25260: Throw exception on metadata constraint
violation, instead of retrying
Posted by Josh Elser <jo...@gmail.com>.
> On Sept. 2, 2014, 10:32 p.m., Josh Elser wrote:
> > test/src/test/java/org/apache/accumulo/test/Accumulo3096IT.java, line 53
> > <https://reviews.apache.org/r/25260/diff/1/?file=674070#file674070line53>
> >
> > Would be better as an Assert.equals() call with a message as to why we expect the CVE as the cause.
>
> kturner wrote:
> If its not the expected exception, I like throwing it. Then we see what the stack trace for the unexpected exception which is useful for debugging.
Ok, I can buy getting the stack trace.
- Josh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25260/#review52094
-----------------------------------------------------------
On Sept. 3, 2014, 3:17 p.m., kturner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25260/
> -----------------------------------------------------------
>
> (Updated Sept. 3, 2014, 3:17 p.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-3096
> https://issues.apache.org/jira/browse/ACCUMULO-3096
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> Throw exception on metadata constraint violation, instead of retrying
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/accumulo/core/client/impl/Writer.java d6762e7
> server/base/src/main/java/org/apache/accumulo/server/util/MetadataTableUtil.java 463ca57
> test/src/test/java/org/apache/accumulo/test/MetaConstraintRetryIT.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/25260/diff/
>
>
> Testing
> -------
>
> Ran mvn package w/o incident . Currently running mvn verify.
>
>
> Thanks,
>
> kturner
>
>
Re: Review Request 25260: Throw exception on metadata constraint
violation, instead of retrying
Posted by Mike Drob <md...@mdrob.com>.
> On Sept. 2, 2014, 10:32 p.m., Josh Elser wrote:
> > test/src/test/java/org/apache/accumulo/test/Accumulo3096IT.java, line 34
> > <https://reviews.apache.org/r/25260/diff/1/?file=674070#file674070line34>
> >
> > It'd be much nicer to have a test name that is more meaningful than a ticket. Having the ticket referenced in a comment is useful if I want to find out more, but at first glance it doesn't tell me anything about what it's testing.
Why is it even useful as a comment? It should already be in the commit message and 'git blame' will tell you who and where it came from.
> On Sept. 2, 2014, 10:32 p.m., Josh Elser wrote:
> > server/base/src/main/java/org/apache/accumulo/server/util/MetadataTableUtil.java, line 149
> > <https://reviews.apache.org/r/25260/diff/1/?file=674069#file674069line149>
> >
> > A comment about why we throw a RTE for ConstraintViolationException but none of the others would be nice (in other words, reference the reason these changes are being done).
HBase has a DoNotRetryIOException, maybe something like that would be interesting to consider.
- Mike
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25260/#review52094
-----------------------------------------------------------
On Sept. 2, 2014, 5:45 p.m., kturner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25260/
> -----------------------------------------------------------
>
> (Updated Sept. 2, 2014, 5:45 p.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-3096
> https://issues.apache.org/jira/browse/ACCUMULO-3096
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> Throw exception on metadata constraint violation, instead of retrying
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/accumulo/core/client/impl/Writer.java d6762e7
> server/base/src/main/java/org/apache/accumulo/server/util/MetadataTableUtil.java 463ca57
> test/src/test/java/org/apache/accumulo/test/Accumulo3096IT.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/25260/diff/
>
>
> Testing
> -------
>
> Ran mvn package w/o incident . Currently running mvn verify.
>
>
> Thanks,
>
> kturner
>
>
Re: Review Request 25260: Throw exception on metadata constraint
violation, instead of retrying
Posted by Josh Elser <jo...@gmail.com>.
> On Sept. 2, 2014, 10:32 p.m., Josh Elser wrote:
> > test/src/test/java/org/apache/accumulo/test/Accumulo3096IT.java, line 34
> > <https://reviews.apache.org/r/25260/diff/1/?file=674070#file674070line34>
> >
> > It'd be much nicer to have a test name that is more meaningful than a ticket. Having the ticket referenced in a comment is useful if I want to find out more, but at first glance it doesn't tell me anything about what it's testing.
>
> Mike Drob wrote:
> Why is it even useful as a comment? It should already be in the commit message and 'git blame' will tell you who and where it came from.
That's valid -- a bit more personal preference, I suppose. I prefer to have information in the code rather than have to look at the history to find when it was introduced, especially when the most recent change isn't from the ticket that originally introduced it (when blame would be wrong).
- Josh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25260/#review52094
-----------------------------------------------------------
On Sept. 2, 2014, 5:45 p.m., kturner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25260/
> -----------------------------------------------------------
>
> (Updated Sept. 2, 2014, 5:45 p.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-3096
> https://issues.apache.org/jira/browse/ACCUMULO-3096
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> Throw exception on metadata constraint violation, instead of retrying
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/accumulo/core/client/impl/Writer.java d6762e7
> server/base/src/main/java/org/apache/accumulo/server/util/MetadataTableUtil.java 463ca57
> test/src/test/java/org/apache/accumulo/test/Accumulo3096IT.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/25260/diff/
>
>
> Testing
> -------
>
> Ran mvn package w/o incident . Currently running mvn verify.
>
>
> Thanks,
>
> kturner
>
>
Re: Review Request 25260: Throw exception on metadata constraint
violation, instead of retrying
Posted by Josh Elser <jo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25260/#review52094
-----------------------------------------------------------
server/base/src/main/java/org/apache/accumulo/server/util/MetadataTableUtil.java
<https://reviews.apache.org/r/25260/#comment90830>
A comment about why we throw a RTE for ConstraintViolationException but none of the others would be nice (in other words, reference the reason these changes are being done).
test/src/test/java/org/apache/accumulo/test/Accumulo3096IT.java
<https://reviews.apache.org/r/25260/#comment90831>
It'd be much nicer to have a test name that is more meaningful than a ticket. Having the ticket referenced in a comment is useful if I want to find out more, but at first glance it doesn't tell me anything about what it's testing.
test/src/test/java/org/apache/accumulo/test/Accumulo3096IT.java
<https://reviews.apache.org/r/25260/#comment90832>
Message for the failure would be nice.
test/src/test/java/org/apache/accumulo/test/Accumulo3096IT.java
<https://reviews.apache.org/r/25260/#comment90833>
Would be better as an Assert.equals() call with a message as to why we expect the CVE as the cause.
- Josh Elser
On Sept. 2, 2014, 5:45 p.m., kturner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25260/
> -----------------------------------------------------------
>
> (Updated Sept. 2, 2014, 5:45 p.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-3096
> https://issues.apache.org/jira/browse/ACCUMULO-3096
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> Throw exception on metadata constraint violation, instead of retrying
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/accumulo/core/client/impl/Writer.java d6762e7
> server/base/src/main/java/org/apache/accumulo/server/util/MetadataTableUtil.java 463ca57
> test/src/test/java/org/apache/accumulo/test/Accumulo3096IT.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/25260/diff/
>
>
> Testing
> -------
>
> Ran mvn package w/o incident . Currently running mvn verify.
>
>
> Thanks,
>
> kturner
>
>