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/12/12 22:32:44 UTC
Review Request 16224: ACCUMULO-1986 - data integrity checks for Key and
Mutation Thrift constructors
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16224/
-----------------------------------------------------------
Review request for accumulo.
Bugs: ACCUMULO-1986
https://issues.apache.org/jira/browse/ACCUMULO-1986
Repository: accumulo
Description
-------
Integrity checks for constructing Key and Mutation objects from Thrift.
To make adding unit tests easier, I updated KeyTest and MutationTest to use JUnit 4 annotations. That drove the need to rename an assertEquals method in MutationTest, hence all the changes to "checkCU" there.
See the JIRA ticket for a list of classes that I evaluated for these same sort of integrity checks.
Diffs
-----
src/core/src/main/java/org/apache/accumulo/core/data/Key.java cfb0b5c
src/core/src/main/java/org/apache/accumulo/core/data/Mutation.java 3979da9
src/core/src/test/java/org/apache/accumulo/core/data/KeyTest.java 9a7f0d7
src/core/src/test/java/org/apache/accumulo/core/data/MutationTest.java 38ddcad
Diff: https://reviews.apache.org/r/16224/diff/
Testing
-------
Unit tests and functional tests pass.
Thanks,
Bill Havanki
Re: Review Request 16224: ACCUMULO-1986 - data integrity checks for Key and
Mutation Thrift constructors
Posted by Josh Elser <jo...@gmail.com>.
> On Dec. 12, 2013, 9:40 p.m., Eric Newton wrote:
> > src/core/src/main/java/org/apache/accumulo/core/data/Key.java, line 295
> > <https://reviews.apache.org/r/16224/diff/1/?file=397000#file397000line295>
> >
> > Check out o.a.a.c.u.ArgumentChecker for null value checks.
>
> Bill Havanki wrote:
> Ugh, the checks are nice but their exception messages aren't customizable, e.g., "argument 'row' was null". I could enhance ArgumentChecker, if you think it's important enough to do that in this ticket.
Agreed, Bill. ArgumentChecker is nicer than nothing, but Guava's Preconditions gives a bunch more flexibility. I'd rather see us move away from ArgumentChecker to Preconditions, but that's definitely for another ticket :)
- Josh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16224/#review30279
-----------------------------------------------------------
On Dec. 12, 2013, 9:32 p.m., Bill Havanki wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16224/
> -----------------------------------------------------------
>
> (Updated Dec. 12, 2013, 9:32 p.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-1986
> https://issues.apache.org/jira/browse/ACCUMULO-1986
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> Integrity checks for constructing Key and Mutation objects from Thrift.
>
> To make adding unit tests easier, I updated KeyTest and MutationTest to use JUnit 4 annotations. That drove the need to rename an assertEquals method in MutationTest, hence all the changes to "checkCU" there.
>
> See the JIRA ticket for a list of classes that I evaluated for these same sort of integrity checks.
>
>
> Diffs
> -----
>
> src/core/src/main/java/org/apache/accumulo/core/data/Key.java cfb0b5c
> src/core/src/main/java/org/apache/accumulo/core/data/Mutation.java 3979da9
> src/core/src/test/java/org/apache/accumulo/core/data/KeyTest.java 9a7f0d7
> src/core/src/test/java/org/apache/accumulo/core/data/MutationTest.java 38ddcad
>
> Diff: https://reviews.apache.org/r/16224/diff/
>
>
> Testing
> -------
>
> Unit tests and functional tests pass.
>
>
> Thanks,
>
> Bill Havanki
>
>
Re: Review Request 16224: ACCUMULO-1986 - data integrity checks for Key and
Mutation Thrift constructors
Posted by Bill Havanki <bh...@clouderagovt.com>.
> On Dec. 12, 2013, 4:40 p.m., Eric Newton wrote:
> > src/core/src/main/java/org/apache/accumulo/core/data/Key.java, line 295
> > <https://reviews.apache.org/r/16224/diff/1/?file=397000#file397000line295>
> >
> > Check out o.a.a.c.u.ArgumentChecker for null value checks.
Ugh, the checks are nice but their exception messages aren't customizable, e.g., "argument 'row' was null". I could enhance ArgumentChecker, if you think it's important enough to do that in this ticket.
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16224/#review30279
-----------------------------------------------------------
On Dec. 12, 2013, 4:32 p.m., Bill Havanki wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16224/
> -----------------------------------------------------------
>
> (Updated Dec. 12, 2013, 4:32 p.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-1986
> https://issues.apache.org/jira/browse/ACCUMULO-1986
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> Integrity checks for constructing Key and Mutation objects from Thrift.
>
> To make adding unit tests easier, I updated KeyTest and MutationTest to use JUnit 4 annotations. That drove the need to rename an assertEquals method in MutationTest, hence all the changes to "checkCU" there.
>
> See the JIRA ticket for a list of classes that I evaluated for these same sort of integrity checks.
>
>
> Diffs
> -----
>
> src/core/src/main/java/org/apache/accumulo/core/data/Key.java cfb0b5c
> src/core/src/main/java/org/apache/accumulo/core/data/Mutation.java 3979da9
> src/core/src/test/java/org/apache/accumulo/core/data/KeyTest.java 9a7f0d7
> src/core/src/test/java/org/apache/accumulo/core/data/MutationTest.java 38ddcad
>
> Diff: https://reviews.apache.org/r/16224/diff/
>
>
> Testing
> -------
>
> Unit tests and functional tests pass.
>
>
> Thanks,
>
> Bill Havanki
>
>
Re: Review Request 16224: ACCUMULO-1986 - data integrity checks for Key and
Mutation Thrift constructors
Posted by John Vines <vi...@apache.org>.
> On Dec. 12, 2013, 9:40 p.m., Eric Newton wrote:
> > src/core/src/main/java/org/apache/accumulo/core/data/Key.java, line 295
> > <https://reviews.apache.org/r/16224/diff/1/?file=397000#file397000line295>
> >
> > Check out o.a.a.c.u.ArgumentChecker for null value checks.
>
> Bill Havanki wrote:
> Ugh, the checks are nice but their exception messages aren't customizable, e.g., "argument 'row' was null". I could enhance ArgumentChecker, if you think it's important enough to do that in this ticket.
>
> Josh Elser wrote:
> Agreed, Bill. ArgumentChecker is nicer than nothing, but Guava's Preconditions gives a bunch more flexibility. I'd rather see us move away from ArgumentChecker to Preconditions, but that's definitely for another ticket :)
Why don't you go ahead and put in a ticket for that, Josh. I think there's a lot we can offload onto Guava and this is one that I wasn't aware of that would be worthwhile.
- John
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16224/#review30279
-----------------------------------------------------------
On Dec. 16, 2013, 3:47 p.m., Bill Havanki wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16224/
> -----------------------------------------------------------
>
> (Updated Dec. 16, 2013, 3:47 p.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-1986
> https://issues.apache.org/jira/browse/ACCUMULO-1986
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> Integrity checks for constructing Key and Mutation objects from Thrift.
>
> To make adding unit tests easier, I updated KeyTest and MutationTest to use JUnit 4 annotations. That drove the need to rename an assertEquals method in MutationTest, hence all the changes to "checkCU" there.
>
> See the JIRA ticket for a list of classes that I evaluated for these same sort of integrity checks.
>
>
> Diffs
> -----
>
> src/core/src/main/java/org/apache/accumulo/core/data/Key.java cfb0b5c
> src/core/src/main/java/org/apache/accumulo/core/data/Mutation.java 3979da9
> src/core/src/test/java/org/apache/accumulo/core/data/KeyTest.java 9a7f0d7
> src/core/src/test/java/org/apache/accumulo/core/data/MutationTest.java 38ddcad
>
> Diff: https://reviews.apache.org/r/16224/diff/
>
>
> Testing
> -------
>
> Unit tests and functional tests pass.
>
>
> Thanks,
>
> Bill Havanki
>
>
Re: Review Request 16224: ACCUMULO-1986 - data integrity checks for Key and
Mutation Thrift constructors
Posted by Bill Havanki <bh...@clouderagovt.com>.
> On Dec. 12, 2013, 4:40 p.m., Eric Newton wrote:
> > src/core/src/main/java/org/apache/accumulo/core/data/Key.java, line 295
> > <https://reviews.apache.org/r/16224/diff/1/?file=397000#file397000line295>
> >
> > Check out o.a.a.c.u.ArgumentChecker for null value checks.
>
> Bill Havanki wrote:
> Ugh, the checks are nice but their exception messages aren't customizable, e.g., "argument 'row' was null". I could enhance ArgumentChecker, if you think it's important enough to do that in this ticket.
>
> Josh Elser wrote:
> Agreed, Bill. ArgumentChecker is nicer than nothing, but Guava's Preconditions gives a bunch more flexibility. I'd rather see us move away from ArgumentChecker to Preconditions, but that's definitely for another ticket :)
>
> John Vines wrote:
> Why don't you go ahead and put in a ticket for that, Josh. I think there's a lot we can offload onto Guava and this is one that I wasn't aware of that would be worthwhile.
I will drop this issue for now in favor of ACCUMULO-2021, filed by Josh.
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16224/#review30279
-----------------------------------------------------------
On Dec. 16, 2013, 10:47 a.m., Bill Havanki wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16224/
> -----------------------------------------------------------
>
> (Updated Dec. 16, 2013, 10:47 a.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-1986
> https://issues.apache.org/jira/browse/ACCUMULO-1986
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> Integrity checks for constructing Key and Mutation objects from Thrift.
>
> To make adding unit tests easier, I updated KeyTest and MutationTest to use JUnit 4 annotations. That drove the need to rename an assertEquals method in MutationTest, hence all the changes to "checkCU" there.
>
> See the JIRA ticket for a list of classes that I evaluated for these same sort of integrity checks.
>
>
> Diffs
> -----
>
> src/core/src/main/java/org/apache/accumulo/core/data/Key.java cfb0b5c
> src/core/src/main/java/org/apache/accumulo/core/data/Mutation.java 3979da9
> src/core/src/test/java/org/apache/accumulo/core/data/KeyTest.java 9a7f0d7
> src/core/src/test/java/org/apache/accumulo/core/data/MutationTest.java 38ddcad
>
> Diff: https://reviews.apache.org/r/16224/diff/
>
>
> Testing
> -------
>
> Unit tests and functional tests pass.
>
>
> Thanks,
>
> Bill Havanki
>
>
Re: Review Request 16224: ACCUMULO-1986 - data integrity checks for Key and
Mutation Thrift constructors
Posted by Eric Newton <er...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16224/#review30279
-----------------------------------------------------------
src/core/src/main/java/org/apache/accumulo/core/data/Key.java
<https://reviews.apache.org/r/16224/#comment57969>
Check out o.a.a.c.u.ArgumentChecker for null value checks.
- Eric Newton
On Dec. 12, 2013, 9:32 p.m., Bill Havanki wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16224/
> -----------------------------------------------------------
>
> (Updated Dec. 12, 2013, 9:32 p.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-1986
> https://issues.apache.org/jira/browse/ACCUMULO-1986
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> Integrity checks for constructing Key and Mutation objects from Thrift.
>
> To make adding unit tests easier, I updated KeyTest and MutationTest to use JUnit 4 annotations. That drove the need to rename an assertEquals method in MutationTest, hence all the changes to "checkCU" there.
>
> See the JIRA ticket for a list of classes that I evaluated for these same sort of integrity checks.
>
>
> Diffs
> -----
>
> src/core/src/main/java/org/apache/accumulo/core/data/Key.java cfb0b5c
> src/core/src/main/java/org/apache/accumulo/core/data/Mutation.java 3979da9
> src/core/src/test/java/org/apache/accumulo/core/data/KeyTest.java 9a7f0d7
> src/core/src/test/java/org/apache/accumulo/core/data/MutationTest.java 38ddcad
>
> Diff: https://reviews.apache.org/r/16224/diff/
>
>
> Testing
> -------
>
> Unit tests and functional tests pass.
>
>
> Thanks,
>
> Bill Havanki
>
>
Re: Review Request 16224: ACCUMULO-1986 - data integrity checks for Key and
Mutation Thrift constructors
Posted by Eric Newton <er...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16224/#review30450
-----------------------------------------------------------
Ship it!
Ship It!
- Eric Newton
On Dec. 16, 2013, 3:47 p.m., Bill Havanki wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16224/
> -----------------------------------------------------------
>
> (Updated Dec. 16, 2013, 3:47 p.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-1986
> https://issues.apache.org/jira/browse/ACCUMULO-1986
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> Integrity checks for constructing Key and Mutation objects from Thrift.
>
> To make adding unit tests easier, I updated KeyTest and MutationTest to use JUnit 4 annotations. That drove the need to rename an assertEquals method in MutationTest, hence all the changes to "checkCU" there.
>
> See the JIRA ticket for a list of classes that I evaluated for these same sort of integrity checks.
>
>
> Diffs
> -----
>
> src/core/src/main/java/org/apache/accumulo/core/data/Key.java cfb0b5c
> src/core/src/main/java/org/apache/accumulo/core/data/Mutation.java 3979da9
> src/core/src/test/java/org/apache/accumulo/core/data/KeyTest.java 9a7f0d7
> src/core/src/test/java/org/apache/accumulo/core/data/MutationTest.java 38ddcad
>
> Diff: https://reviews.apache.org/r/16224/diff/
>
>
> Testing
> -------
>
> Unit tests and functional tests pass.
>
>
> Thanks,
>
> Bill Havanki
>
>
Re: Review Request 16224: ACCUMULO-1986 - data integrity checks for Key and
Mutation Thrift constructors
Posted by Bill Havanki <bh...@clouderagovt.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16224/
-----------------------------------------------------------
(Updated Dec. 16, 2013, 10:47 a.m.)
Review request for accumulo.
Changes
-------
Mike Drob's suggestions: remove import static *, rename checkCU to verifyColumnUpdate.
Bugs: ACCUMULO-1986
https://issues.apache.org/jira/browse/ACCUMULO-1986
Repository: accumulo
Description
-------
Integrity checks for constructing Key and Mutation objects from Thrift.
To make adding unit tests easier, I updated KeyTest and MutationTest to use JUnit 4 annotations. That drove the need to rename an assertEquals method in MutationTest, hence all the changes to "checkCU" there.
See the JIRA ticket for a list of classes that I evaluated for these same sort of integrity checks.
Diffs (updated)
-----
src/core/src/main/java/org/apache/accumulo/core/data/Key.java cfb0b5c
src/core/src/main/java/org/apache/accumulo/core/data/Mutation.java 3979da9
src/core/src/test/java/org/apache/accumulo/core/data/KeyTest.java 9a7f0d7
src/core/src/test/java/org/apache/accumulo/core/data/MutationTest.java 38ddcad
Diff: https://reviews.apache.org/r/16224/diff/
Testing
-------
Unit tests and functional tests pass.
Thanks,
Bill Havanki
Re: Review Request 16224: ACCUMULO-1986 - data integrity checks for Key and
Mutation Thrift constructors
Posted by Bill Havanki <bh...@clouderagovt.com>.
> On Dec. 13, 2013, 2:16 p.m., Mike Drob wrote:
> > src/core/src/test/java/org/apache/accumulo/core/data/KeyTest.java, line 20
> > <https://reviews.apache.org/r/16224/diff/1/?file=397002#file397002line20>
> >
> > Do we have official guidance against using import *? Regardless, I'd prefer this changed.
Personally, I loosen standards a bit when it comes to unit tests, and particularly for assertXxx methods in unit tests, everyone pretty much knows where they come from. However, I just surveyed the unit tests and hardly any use this construct, so I'll change it to not use *.
> On Dec. 13, 2013, 2:16 p.m., Mike Drob wrote:
> > src/core/src/test/java/org/apache/accumulo/core/data/MutationTest.java, line 317
> > <https://reviews.apache.org/r/16224/diff/1/?file=397003#file397003line317>
> >
> > Instead of renaming the method, another option would have been to qualify references to if as this.assertEquals(...). Not sure which one is preferable. I don't like the name "checkCU" but can't offer any alternatives, so maybe it's fine.
I don't like the assertEquals thing since it implies that it's part of JUnit. Since I will be generating a new patch anyway, if you do think of a better name, I'll add it in.
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16224/#review30359
-----------------------------------------------------------
On Dec. 12, 2013, 4:32 p.m., Bill Havanki wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16224/
> -----------------------------------------------------------
>
> (Updated Dec. 12, 2013, 4:32 p.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-1986
> https://issues.apache.org/jira/browse/ACCUMULO-1986
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> Integrity checks for constructing Key and Mutation objects from Thrift.
>
> To make adding unit tests easier, I updated KeyTest and MutationTest to use JUnit 4 annotations. That drove the need to rename an assertEquals method in MutationTest, hence all the changes to "checkCU" there.
>
> See the JIRA ticket for a list of classes that I evaluated for these same sort of integrity checks.
>
>
> Diffs
> -----
>
> src/core/src/main/java/org/apache/accumulo/core/data/Key.java cfb0b5c
> src/core/src/main/java/org/apache/accumulo/core/data/Mutation.java 3979da9
> src/core/src/test/java/org/apache/accumulo/core/data/KeyTest.java 9a7f0d7
> src/core/src/test/java/org/apache/accumulo/core/data/MutationTest.java 38ddcad
>
> Diff: https://reviews.apache.org/r/16224/diff/
>
>
> Testing
> -------
>
> Unit tests and functional tests pass.
>
>
> Thanks,
>
> Bill Havanki
>
>
Re: Review Request 16224: ACCUMULO-1986 - data integrity checks for Key and
Mutation Thrift constructors
Posted by Mike Drob <md...@mdrob.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16224/#review30359
-----------------------------------------------------------
src/core/src/test/java/org/apache/accumulo/core/data/KeyTest.java
<https://reviews.apache.org/r/16224/#comment58102>
Do we have official guidance against using import *? Regardless, I'd prefer this changed.
src/core/src/test/java/org/apache/accumulo/core/data/MutationTest.java
<https://reviews.apache.org/r/16224/#comment58103>
Do we have official guidance against using import *? Regardless, I'd prefer this changed.
src/core/src/test/java/org/apache/accumulo/core/data/MutationTest.java
<https://reviews.apache.org/r/16224/#comment58104>
Instead of renaming the method, another option would have been to qualify references to if as this.assertEquals(...). Not sure which one is preferable. I don't like the name "checkCU" but can't offer any alternatives, so maybe it's fine.
- Mike Drob
On Dec. 12, 2013, 9:32 p.m., Bill Havanki wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16224/
> -----------------------------------------------------------
>
> (Updated Dec. 12, 2013, 9:32 p.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-1986
> https://issues.apache.org/jira/browse/ACCUMULO-1986
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> Integrity checks for constructing Key and Mutation objects from Thrift.
>
> To make adding unit tests easier, I updated KeyTest and MutationTest to use JUnit 4 annotations. That drove the need to rename an assertEquals method in MutationTest, hence all the changes to "checkCU" there.
>
> See the JIRA ticket for a list of classes that I evaluated for these same sort of integrity checks.
>
>
> Diffs
> -----
>
> src/core/src/main/java/org/apache/accumulo/core/data/Key.java cfb0b5c
> src/core/src/main/java/org/apache/accumulo/core/data/Mutation.java 3979da9
> src/core/src/test/java/org/apache/accumulo/core/data/KeyTest.java 9a7f0d7
> src/core/src/test/java/org/apache/accumulo/core/data/MutationTest.java 38ddcad
>
> Diff: https://reviews.apache.org/r/16224/diff/
>
>
> Testing
> -------
>
> Unit tests and functional tests pass.
>
>
> Thanks,
>
> Bill Havanki
>
>