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