accumulo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From ke...@deenlo.com
Subject Re: Review Request 25260: Throw exception on metadata constraint violation, instead of retrying
Date Wed, 03 Sep 2014 14:40:50 GMT


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


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