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