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/06 17:02:22 UTC
Review Request 16081: ACCUMULO-1958 - Safer Range constructor
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16081/
-----------------------------------------------------------
Review request for accumulo.
Bugs: ACCUMULO-1958
https://issues.apache.org/jira/browse/ACCUMULO-1958
Repository: accumulo
Description
-------
Based on Sean's strategy, creates a new protected Range constructor without the start/stop key check, and adds the check to the public six-argument constructor. Opted not to deprecate the public constructor at this time, since it is now safe.
Diffs
-----
src/core/src/main/java/org/apache/accumulo/core/data/Range.java 7ef0dc5710877cdd0dd3ead69e7db5d8c9ef68c1
Diff: https://reviews.apache.org/r/16081/diff/
Testing
-------
Unit testing for Range still passes.
Thanks,
Bill Havanki
Re: Review Request 16081: ACCUMULO-1958 - Safer Range constructor
Posted by ke...@deenlo.com.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16081/#review30260
-----------------------------------------------------------
Ship it!
Ship It!
- kturner
On Dec. 9, 2013, 4:18 p.m., Bill Havanki wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16081/
> -----------------------------------------------------------
>
> (Updated Dec. 9, 2013, 4:18 p.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-1958
> https://issues.apache.org/jira/browse/ACCUMULO-1958
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> Based on Sean's strategy, creates a new protected Range constructor without the start/stop key check, and adds the check to the public six-argument constructor. Opted not to deprecate the public constructor at this time, since it is now safe.
>
>
> Diffs
> -----
>
> src/core/src/main/java/org/apache/accumulo/core/data/Range.java 7ef0dc5710877cdd0dd3ead69e7db5d8c9ef68c1
> src/core/src/test/java/org/apache/accumulo/core/data/RangeTest.java a8d91b0944cdab6ff10ec0847fe999f0ce666631
>
> Diff: https://reviews.apache.org/r/16081/diff/
>
>
> Testing
> -------
>
> Unit testing for Range still passes.
>
>
> Thanks,
>
> Bill Havanki
>
>
Re: Review Request 16081: ACCUMULO-1958 - Safer Range constructor
Posted by Bill Havanki <bh...@clouderagovt.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16081/
-----------------------------------------------------------
(Updated Dec. 9, 2013, 11:18 a.m.)
Review request for accumulo.
Changes
-------
Added validity checks to Thrift constructor and readFields. Javadoc can be added as part of ACCUMULO-1931, which is in flight (review: https://reviews.apache.org/r/15857/).
Bugs: ACCUMULO-1958
https://issues.apache.org/jira/browse/ACCUMULO-1958
Repository: accumulo
Description
-------
Based on Sean's strategy, creates a new protected Range constructor without the start/stop key check, and adds the check to the public six-argument constructor. Opted not to deprecate the public constructor at this time, since it is now safe.
Diffs (updated)
-----
src/core/src/main/java/org/apache/accumulo/core/data/Range.java 7ef0dc5710877cdd0dd3ead69e7db5d8c9ef68c1
src/core/src/test/java/org/apache/accumulo/core/data/RangeTest.java a8d91b0944cdab6ff10ec0847fe999f0ce666631
Diff: https://reviews.apache.org/r/16081/diff/
Testing
-------
Unit testing for Range still passes.
Thanks,
Bill Havanki
Re: Review Request 16081: ACCUMULO-1958 - Safer Range constructor
Posted by ke...@deenlo.com.
> On Dec. 6, 2013, 8:35 p.m., kturner wrote:
> > src/core/src/main/java/org/apache/accumulo/core/data/Range.java, line 236
> > <https://reviews.apache.org/r/16081/diff/2/?file=395097#file395097line236>
> >
> > I am thinking it might be a good idea to do the check beforeStartKey() check for this constructor and the readFields() method. Thinking about the case of deserializing corrupted data. readFields() is used in a similar way to a constructor.
>
> kturner wrote:
> I am not sure if this is worth doing. Just something I thought of. Given enough time and and enough machines this sanity check in deserialization would eventually catch an error. But it comes at the cost of doing the check that was already done on the client side and assured by crcs at lower levels. There is also the possibility that users will use these methods directly. I know users write their own serializtion code for Key and Mutaton inorder to use map reduce and pipes.
>
> Bill Havanki wrote:
> I like adding the check in readFields(). It is a form of deserialization, and like you said, equivalent to a constructor, so it should be checking for invalid data. The same thing is usually done in readObject() for Java serialization. I'd like to see a follow-on JIRA to address all the readFields() for the need for data checks, since there are a bunch of 'em.
>
> I don't know enough about Thrift to have a good opinion here. Maybe that's another issue to look at across the classes, and decide whether to address.
>
I think a separate issue for checking all readFields makes sense. Also would be ok to improve it as part of this patch. If the change is added to readFields, then it should be added to the constructor that takes a thrift object. In that case Thrift is deserializing the range data and we would sanity check the data. W/ readFields we would be deserializing and sanity checking.
- kturner
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16081/#review29907
-----------------------------------------------------------
On Dec. 6, 2013, 5:06 p.m., Bill Havanki wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16081/
> -----------------------------------------------------------
>
> (Updated Dec. 6, 2013, 5:06 p.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-1958
> https://issues.apache.org/jira/browse/ACCUMULO-1958
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> Based on Sean's strategy, creates a new protected Range constructor without the start/stop key check, and adds the check to the public six-argument constructor. Opted not to deprecate the public constructor at this time, since it is now safe.
>
>
> Diffs
> -----
>
> src/core/src/main/java/org/apache/accumulo/core/data/Range.java 7ef0dc5710877cdd0dd3ead69e7db5d8c9ef68c1
>
> Diff: https://reviews.apache.org/r/16081/diff/
>
>
> Testing
> -------
>
> Unit testing for Range still passes.
>
>
> Thanks,
>
> Bill Havanki
>
>
Re: Review Request 16081: ACCUMULO-1958 - Safer Range constructor
Posted by Bill Havanki <bh...@clouderagovt.com>.
> On Dec. 6, 2013, 3:35 p.m., kturner wrote:
> > src/core/src/main/java/org/apache/accumulo/core/data/Range.java, line 236
> > <https://reviews.apache.org/r/16081/diff/2/?file=395097#file395097line236>
> >
> > I am thinking it might be a good idea to do the check beforeStartKey() check for this constructor and the readFields() method. Thinking about the case of deserializing corrupted data. readFields() is used in a similar way to a constructor.
>
> kturner wrote:
> I am not sure if this is worth doing. Just something I thought of. Given enough time and and enough machines this sanity check in deserialization would eventually catch an error. But it comes at the cost of doing the check that was already done on the client side and assured by crcs at lower levels. There is also the possibility that users will use these methods directly. I know users write their own serializtion code for Key and Mutaton inorder to use map reduce and pipes.
I like adding the check in readFields(). It is a form of deserialization, and like you said, equivalent to a constructor, so it should be checking for invalid data. The same thing is usually done in readObject() for Java serialization. I'd like to see a follow-on JIRA to address all the readFields() for the need for data checks, since there are a bunch of 'em.
I don't know enough about Thrift to have a good opinion here. Maybe that's another issue to look at across the classes, and decide whether to address.
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16081/#review29907
-----------------------------------------------------------
On Dec. 6, 2013, 12:06 p.m., Bill Havanki wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16081/
> -----------------------------------------------------------
>
> (Updated Dec. 6, 2013, 12:06 p.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-1958
> https://issues.apache.org/jira/browse/ACCUMULO-1958
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> Based on Sean's strategy, creates a new protected Range constructor without the start/stop key check, and adds the check to the public six-argument constructor. Opted not to deprecate the public constructor at this time, since it is now safe.
>
>
> Diffs
> -----
>
> src/core/src/main/java/org/apache/accumulo/core/data/Range.java 7ef0dc5710877cdd0dd3ead69e7db5d8c9ef68c1
>
> Diff: https://reviews.apache.org/r/16081/diff/
>
>
> Testing
> -------
>
> Unit testing for Range still passes.
>
>
> Thanks,
>
> Bill Havanki
>
>
Re: Review Request 16081: ACCUMULO-1958 - Safer Range constructor
Posted by ke...@deenlo.com.
> On Dec. 6, 2013, 8:35 p.m., kturner wrote:
> > src/core/src/main/java/org/apache/accumulo/core/data/Range.java, line 236
> > <https://reviews.apache.org/r/16081/diff/2/?file=395097#file395097line236>
> >
> > I am thinking it might be a good idea to do the check beforeStartKey() check for this constructor and the readFields() method. Thinking about the case of deserializing corrupted data. readFields() is used in a similar way to a constructor.
>
> kturner wrote:
> I am not sure if this is worth doing. Just something I thought of. Given enough time and and enough machines this sanity check in deserialization would eventually catch an error. But it comes at the cost of doing the check that was already done on the client side and assured by crcs at lower levels. There is also the possibility that users will use these methods directly. I know users write their own serializtion code for Key and Mutaton inorder to use map reduce and pipes.
>
> Bill Havanki wrote:
> I like adding the check in readFields(). It is a form of deserialization, and like you said, equivalent to a constructor, so it should be checking for invalid data. The same thing is usually done in readObject() for Java serialization. I'd like to see a follow-on JIRA to address all the readFields() for the need for data checks, since there are a bunch of 'em.
>
> I don't know enough about Thrift to have a good opinion here. Maybe that's another issue to look at across the classes, and decide whether to address.
>
>
> kturner wrote:
> I think a separate issue for checking all readFields makes sense. Also would be ok to improve it as part of this patch. If the change is added to readFields, then it should be added to the constructor that takes a thrift object. In that case Thrift is deserializing the range data and we would sanity check the data. W/ readFields we would be deserializing and sanity checking.
>
> Bill Havanki wrote:
> Sounds good, I'll make the changes for Range here. Would you prefer one JIRA for sanity checks in both readFields and Thrift deserialization, or one for each? (I'm thinking a single one, since the cases are so similar.)
I think a single issue sounds good also for the same reason
- kturner
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16081/#review29907
-----------------------------------------------------------
On Dec. 6, 2013, 5:06 p.m., Bill Havanki wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16081/
> -----------------------------------------------------------
>
> (Updated Dec. 6, 2013, 5:06 p.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-1958
> https://issues.apache.org/jira/browse/ACCUMULO-1958
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> Based on Sean's strategy, creates a new protected Range constructor without the start/stop key check, and adds the check to the public six-argument constructor. Opted not to deprecate the public constructor at this time, since it is now safe.
>
>
> Diffs
> -----
>
> src/core/src/main/java/org/apache/accumulo/core/data/Range.java 7ef0dc5710877cdd0dd3ead69e7db5d8c9ef68c1
>
> Diff: https://reviews.apache.org/r/16081/diff/
>
>
> Testing
> -------
>
> Unit testing for Range still passes.
>
>
> Thanks,
>
> Bill Havanki
>
>
Re: Review Request 16081: ACCUMULO-1958 - Safer Range constructor
Posted by Bill Havanki <bh...@clouderagovt.com>.
> On Dec. 6, 2013, 3:35 p.m., kturner wrote:
> > src/core/src/main/java/org/apache/accumulo/core/data/Range.java, line 236
> > <https://reviews.apache.org/r/16081/diff/2/?file=395097#file395097line236>
> >
> > I am thinking it might be a good idea to do the check beforeStartKey() check for this constructor and the readFields() method. Thinking about the case of deserializing corrupted data. readFields() is used in a similar way to a constructor.
>
> kturner wrote:
> I am not sure if this is worth doing. Just something I thought of. Given enough time and and enough machines this sanity check in deserialization would eventually catch an error. But it comes at the cost of doing the check that was already done on the client side and assured by crcs at lower levels. There is also the possibility that users will use these methods directly. I know users write their own serializtion code for Key and Mutaton inorder to use map reduce and pipes.
>
> Bill Havanki wrote:
> I like adding the check in readFields(). It is a form of deserialization, and like you said, equivalent to a constructor, so it should be checking for invalid data. The same thing is usually done in readObject() for Java serialization. I'd like to see a follow-on JIRA to address all the readFields() for the need for data checks, since there are a bunch of 'em.
>
> I don't know enough about Thrift to have a good opinion here. Maybe that's another issue to look at across the classes, and decide whether to address.
>
>
> kturner wrote:
> I think a separate issue for checking all readFields makes sense. Also would be ok to improve it as part of this patch. If the change is added to readFields, then it should be added to the constructor that takes a thrift object. In that case Thrift is deserializing the range data and we would sanity check the data. W/ readFields we would be deserializing and sanity checking.
Sounds good, I'll make the changes for Range here. Would you prefer one JIRA for sanity checks in both readFields and Thrift deserialization, or one for each? (I'm thinking a single one, since the cases are so similar.)
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16081/#review29907
-----------------------------------------------------------
On Dec. 6, 2013, 12:06 p.m., Bill Havanki wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16081/
> -----------------------------------------------------------
>
> (Updated Dec. 6, 2013, 12:06 p.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-1958
> https://issues.apache.org/jira/browse/ACCUMULO-1958
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> Based on Sean's strategy, creates a new protected Range constructor without the start/stop key check, and adds the check to the public six-argument constructor. Opted not to deprecate the public constructor at this time, since it is now safe.
>
>
> Diffs
> -----
>
> src/core/src/main/java/org/apache/accumulo/core/data/Range.java 7ef0dc5710877cdd0dd3ead69e7db5d8c9ef68c1
>
> Diff: https://reviews.apache.org/r/16081/diff/
>
>
> Testing
> -------
>
> Unit testing for Range still passes.
>
>
> Thanks,
>
> Bill Havanki
>
>
Re: Review Request 16081: ACCUMULO-1958 - Safer Range constructor
Posted by ke...@deenlo.com.
> On Dec. 6, 2013, 8:35 p.m., kturner wrote:
> > src/core/src/main/java/org/apache/accumulo/core/data/Range.java, line 236
> > <https://reviews.apache.org/r/16081/diff/2/?file=395097#file395097line236>
> >
> > I am thinking it might be a good idea to do the check beforeStartKey() check for this constructor and the readFields() method. Thinking about the case of deserializing corrupted data. readFields() is used in a similar way to a constructor.
I am not sure if this is worth doing. Just something I thought of. Given enough time and and enough machines this sanity check in deserialization would eventually catch an error. But it comes at the cost of doing the check that was already done on the client side and assured by crcs at lower levels. There is also the possibility that users will use these methods directly. I know users write their own serializtion code for Key and Mutaton inorder to use map reduce and pipes.
- kturner
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16081/#review29907
-----------------------------------------------------------
On Dec. 6, 2013, 5:06 p.m., Bill Havanki wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16081/
> -----------------------------------------------------------
>
> (Updated Dec. 6, 2013, 5:06 p.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-1958
> https://issues.apache.org/jira/browse/ACCUMULO-1958
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> Based on Sean's strategy, creates a new protected Range constructor without the start/stop key check, and adds the check to the public six-argument constructor. Opted not to deprecate the public constructor at this time, since it is now safe.
>
>
> Diffs
> -----
>
> src/core/src/main/java/org/apache/accumulo/core/data/Range.java 7ef0dc5710877cdd0dd3ead69e7db5d8c9ef68c1
>
> Diff: https://reviews.apache.org/r/16081/diff/
>
>
> Testing
> -------
>
> Unit testing for Range still passes.
>
>
> Thanks,
>
> Bill Havanki
>
>
Re: Review Request 16081: ACCUMULO-1958 - Safer Range constructor
Posted by ke...@deenlo.com.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16081/#review29907
-----------------------------------------------------------
src/core/src/main/java/org/apache/accumulo/core/data/Range.java
<https://reviews.apache.org/r/16081/#comment57384>
I am thinking it might be a good idea to do the check beforeStartKey() check for this constructor and the readFields() method. Thinking about the case of deserializing corrupted data. readFields() is used in a similar way to a constructor.
- kturner
On Dec. 6, 2013, 5:06 p.m., Bill Havanki wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16081/
> -----------------------------------------------------------
>
> (Updated Dec. 6, 2013, 5:06 p.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-1958
> https://issues.apache.org/jira/browse/ACCUMULO-1958
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> Based on Sean's strategy, creates a new protected Range constructor without the start/stop key check, and adds the check to the public six-argument constructor. Opted not to deprecate the public constructor at this time, since it is now safe.
>
>
> Diffs
> -----
>
> src/core/src/main/java/org/apache/accumulo/core/data/Range.java 7ef0dc5710877cdd0dd3ead69e7db5d8c9ef68c1
>
> Diff: https://reviews.apache.org/r/16081/diff/
>
>
> Testing
> -------
>
> Unit testing for Range still passes.
>
>
> Thanks,
>
> Bill Havanki
>
>
Re: Review Request 16081: ACCUMULO-1958 - Safer Range constructor
Posted by Sean Busbey <se...@manvsbeard.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16081/#review29885
-----------------------------------------------------------
Ship it!
Ship It!
- Sean Busbey
On Dec. 6, 2013, 5:06 p.m., Bill Havanki wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16081/
> -----------------------------------------------------------
>
> (Updated Dec. 6, 2013, 5:06 p.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-1958
> https://issues.apache.org/jira/browse/ACCUMULO-1958
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> Based on Sean's strategy, creates a new protected Range constructor without the start/stop key check, and adds the check to the public six-argument constructor. Opted not to deprecate the public constructor at this time, since it is now safe.
>
>
> Diffs
> -----
>
> src/core/src/main/java/org/apache/accumulo/core/data/Range.java 7ef0dc5710877cdd0dd3ead69e7db5d8c9ef68c1
>
> Diff: https://reviews.apache.org/r/16081/diff/
>
>
> Testing
> -------
>
> Unit testing for Range still passes.
>
>
> Thanks,
>
> Bill Havanki
>
>
Re: Review Request 16081: ACCUMULO-1958 - Safer Range constructor
Posted by Bill Havanki <bh...@clouderagovt.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16081/
-----------------------------------------------------------
(Updated Dec. 6, 2013, 12:06 p.m.)
Review request for accumulo.
Changes
-------
Resolving Sean's issues.
Bugs: ACCUMULO-1958
https://issues.apache.org/jira/browse/ACCUMULO-1958
Repository: accumulo
Description
-------
Based on Sean's strategy, creates a new protected Range constructor without the start/stop key check, and adds the check to the public six-argument constructor. Opted not to deprecate the public constructor at this time, since it is now safe.
Diffs (updated)
-----
src/core/src/main/java/org/apache/accumulo/core/data/Range.java 7ef0dc5710877cdd0dd3ead69e7db5d8c9ef68c1
Diff: https://reviews.apache.org/r/16081/diff/
Testing
-------
Unit testing for Range still passes.
Thanks,
Bill Havanki
Re: Review Request 16081: ACCUMULO-1958 - Safer Range constructor
Posted by Bill Havanki <bh...@clouderagovt.com>.
> On Dec. 6, 2013, 11:33 a.m., Sean Busbey wrote:
> > src/core/src/main/java/org/apache/accumulo/core/data/Range.java, lines 183-187
> > <https://reviews.apache.org/r/16081/diff/1/?file=395076#file395076line183>
> >
> > Nit: could we avoid relocating the function within the file so the changeset is smaller?
Moved it back in diff #2.
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16081/#review29880
-----------------------------------------------------------
On Dec. 6, 2013, 12:06 p.m., Bill Havanki wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16081/
> -----------------------------------------------------------
>
> (Updated Dec. 6, 2013, 12:06 p.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-1958
> https://issues.apache.org/jira/browse/ACCUMULO-1958
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> Based on Sean's strategy, creates a new protected Range constructor without the start/stop key check, and adds the check to the public six-argument constructor. Opted not to deprecate the public constructor at this time, since it is now safe.
>
>
> Diffs
> -----
>
> src/core/src/main/java/org/apache/accumulo/core/data/Range.java 7ef0dc5710877cdd0dd3ead69e7db5d8c9ef68c1
>
> Diff: https://reviews.apache.org/r/16081/diff/
>
>
> Testing
> -------
>
> Unit testing for Range still passes.
>
>
> Thanks,
>
> Bill Havanki
>
>
Re: Review Request 16081: ACCUMULO-1958 - Safer Range constructor
Posted by Sean Busbey <se...@manvsbeard.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16081/#review29880
-----------------------------------------------------------
src/core/src/main/java/org/apache/accumulo/core/data/Range.java
<https://reviews.apache.org/r/16081/#comment57347>
Please add a javadoc, esp since this now throws an IllegalArgumentException where it previously did not.
src/core/src/main/java/org/apache/accumulo/core/data/Range.java
<https://reviews.apache.org/r/16081/#comment57348>
Nit: could we avoid relocating the function within the file so the changeset is smaller?
src/core/src/main/java/org/apache/accumulo/core/data/Range.java
<https://reviews.apache.org/r/16081/#comment57346>
We need a javadoc here that explains the reasoning for the additional method.
Otherwise, this is going to get blown away in a refactoring because it looks like it's just a rearrangement of the arg order on Range(Key,Key,boolean,boolean,boolean,boolean)
- Sean Busbey
On Dec. 6, 2013, 4:02 p.m., Bill Havanki wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16081/
> -----------------------------------------------------------
>
> (Updated Dec. 6, 2013, 4:02 p.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-1958
> https://issues.apache.org/jira/browse/ACCUMULO-1958
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> Based on Sean's strategy, creates a new protected Range constructor without the start/stop key check, and adds the check to the public six-argument constructor. Opted not to deprecate the public constructor at this time, since it is now safe.
>
>
> Diffs
> -----
>
> src/core/src/main/java/org/apache/accumulo/core/data/Range.java 7ef0dc5710877cdd0dd3ead69e7db5d8c9ef68c1
>
> Diff: https://reviews.apache.org/r/16081/diff/
>
>
> Testing
> -------
>
> Unit testing for Range still passes.
>
>
> Thanks,
>
> Bill Havanki
>
>