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