> On 2011-06-13 18:23:35, Matt Foley wrote: > > Sorry if this is out of context, but is it really best to also return a > > null here? Shouldn't it check for null result from getSerialization(), > > then throw a (non-NPE) exception? Or do you prefer to do that check and > > throw at a higher level of the code?
I thought about this while doing the review... my thinking was that our other similar APIs do return null -eg CompressionCodecFactory.getCodecByName() returns null if the specified codec isn't found. This API is also marked as LimitedPrivate Evolving so it shouldn't be a problematic breaking change. Maybe we should add a bit of javadoc saying "@returns null if no serializer is known for the given class." ? - Todd ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/884/#review817 ----------------------------------------------------------- On 2011-06-11 22:10:17, Harsh J wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/884/ > ----------------------------------------------------------- > > (Updated 2011-06-11 22:10:17) > > > Review request for hadoop-common and Todd Lipcon. > > > Summary > ------- > > Since getSerialization() can possibly return a null, it is only right that > getSerializer() and getDeserializer() usage functions do the same, instead of > throwing up NPEs. > > Related issue to which this improvement is required: > https://issues.apache.org/jira/browse/MAPREDUCE-2584 > > > This addresses bug HADOOP-7328. > http://issues.apache.org/jira/browse/HADOOP-7328 > > > Diffs > ----- > > src/java/org/apache/hadoop/io/serializer/SerializationFactory.java dee314a > > Diff: https://reviews.apache.org/r/884/diff > > > Testing > ------- > > Existing SequenceFile serialization factory tests pass. The change is merely > to make the functions return null instead of throwing an NPE within. > > > Thanks, > > Harsh > >
