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