Hadoop Common
  1. Hadoop Common
  2. HADOOP-7328

When a serializer class is missing, return null, not throw an NPE.

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.20.2
    • Fix Version/s: 0.23.0
    • Component/s: io
    • Labels:
    • Hadoop Flags:
      Reviewed
    • Tags:
      logging, exception, serializers, serializationfactory

      Description

      When you have a key/value class that's non Writable and you forget to attach io.serializers for the same, an NPE is thrown by the tasks with no information on why or what's missing and what led to it. I think a better exception can be thrown by SerializationFactory instead of an NPE when a class is not found accepted by any of the loaded ones.

      1. HADOOP-7328.r7.diff
        6 kB
        Harsh J
      2. HADOOP-7328.r6.diff
        5 kB
        Harsh J
      3. HADOOP-7328.r5.diff
        5 kB
        Harsh J
      4. HADOOP-7328.r4.diff
        4 kB
        Harsh J
      5. HADOOP-7328.r4.diff
        4 kB
        Harsh J
      6. HADOOP-7328.r3.diff
        4 kB
        Harsh J
      7. HADOOP-7328.r2.diff
        3 kB
        Harsh J
      8. HADOOP-7328.r1.diff
        1 kB
        Harsh J
      9. 0.23-HADOOP-7328.r7.diff
        6 kB
        Harsh J
      10. 0.20-security-HADOOP-7328.r7.diff
        3 kB
        Harsh J

        Issue Links

          Activity

          Hide
          Harsh J added a comment -

          Patch that adds a FATAL log when a null is about to be received for a requested serializer.

          Show
          Harsh J added a comment - Patch that adds a FATAL log when a null is about to be received for a requested serializer.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12480231/HADOOP-7328.r1.diff
          against trunk revision 1126719.

          +1 @author. The patch does not contain any @author tags.

          -1 tests included. The patch doesn't appear to include any new or modified tests.
          Please justify why no new tests are needed for this patch.
          Also please list what manual steps were performed to verify this patch.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed these core unit tests:

          -1 system test framework. The patch failed system test framework compile.

          Test results: https://builds.apache.org/hudson/job/PreCommit-HADOOP-Build/513//testReport/
          Findbugs warnings: https://builds.apache.org/hudson/job/PreCommit-HADOOP-Build/513//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/hudson/job/PreCommit-HADOOP-Build/513//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12480231/HADOOP-7328.r1.diff against trunk revision 1126719. +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these core unit tests: -1 system test framework. The patch failed system test framework compile. Test results: https://builds.apache.org/hudson/job/PreCommit-HADOOP-Build/513//testReport/ Findbugs warnings: https://builds.apache.org/hudson/job/PreCommit-HADOOP-Build/513//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/hudson/job/PreCommit-HADOOP-Build/513//console This message is automatically generated.
          Hide
          Harsh J added a comment -

          Justifications: A LOG addition, does not require a test case addition/change.

          The other seemingly irrelevant change is a checkstyle fix, the line was using a tab char.

          Show
          Harsh J added a comment - Justifications: A LOG addition, does not require a test case addition/change. The other seemingly irrelevant change is a checkstyle fix, the line was using a tab char.
          Hide
          Todd Lipcon added a comment -

          Maybe instead of a new log, we could have the tasks throw a more useful exception?

          Also, would it be possible to detect this issue at submission time? That's much more user friendly than making the user look through failed task logs.

          Show
          Todd Lipcon added a comment - Maybe instead of a new log, we could have the tasks throw a more useful exception? Also, would it be possible to detect this issue at submission time? That's much more user friendly than making the user look through failed task logs.
          Hide
          Harsh J added a comment -

          I thought of the throws XYZException way, but that'd require an incompatible change wouldn't it?

          I think it should be possible to detect the issue at sub-time, but I'll have to try that with some tests. Let me get back to you with something in that direction.

          Show
          Harsh J added a comment - I thought of the throws XYZException way, but that'd require an incompatible change wouldn't it? I think it should be possible to detect the issue at sub-time, but I'll have to try that with some tests. Let me get back to you with something in that direction.
          Hide
          Todd Lipcon added a comment -

          I don't think it's an incompatible change to replace what's currently a NullPointerException with something more descriptive. If the method signature can't throw an IOException, at least a RuntimeException with a nice error message would be an improvement. (To be clear, I'm not talking about changing the serialization factory, but rather the point in MR where the NPE gets thrown because of the null serializer)

          Show
          Todd Lipcon added a comment - I don't think it's an incompatible change to replace what's currently a NullPointerException with something more descriptive. If the method signature can't throw an IOException, at least a RuntimeException with a nice error message would be an improvement. (To be clear, I'm not talking about changing the serialization factory, but rather the point in MR where the NPE gets thrown because of the null serializer)
          Hide
          jiraposter@reviews.apache.org added a comment -

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          https://reviews.apache.org/r/884/
          -----------------------------------------------------------

          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

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/884/ ----------------------------------------------------------- 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
          Hide
          Harsh J added a comment -

          Please review patch at https://reviews.apache.org/r/884/

          Show
          Harsh J added a comment - Please review patch at https://reviews.apache.org/r/884/
          Hide
          jiraposter@reviews.apache.org added a comment -

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          https://reviews.apache.org/r/884/#review805
          -----------------------------------------------------------

          Ship it!

          Looks good to me. Can you upload this rev of the patch to the JIRA so the QA Bot runs on it?

          • Todd

          On 2011-06-11 22:10:17, Harsh Chouraria 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

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/884/#review805 ----------------------------------------------------------- Ship it! Looks good to me. Can you upload this rev of the patch to the JIRA so the QA Bot runs on it? Todd On 2011-06-11 22:10:17, Harsh Chouraria 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
          Hide
          Harsh J added a comment -

          Reviewed patch attached.

          (+ a couple of bad whitespace fixes)

          Show
          Harsh J added a comment - Reviewed patch attached. (+ a couple of bad whitespace fixes)
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12482273/HADOOP-7328.r2.diff
          against trunk revision 1134861.

          +1 @author. The patch does not contain any @author tags.

          -1 tests included. The patch doesn't appear to include any new or modified tests.
          Please justify why no new tests are needed for this patch.
          Also please list what manual steps were performed to verify this patch.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed core unit tests.

          +1 system test framework. The patch passed system test framework compile.

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/615//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/615//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/615//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12482273/HADOOP-7328.r2.diff against trunk revision 1134861. +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 system test framework. The patch passed system test framework compile. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/615//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/615//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/615//console This message is automatically generated.
          Hide
          jiraposter@reviews.apache.org added a comment -

          On 2011-06-12 02:24:55, Todd Lipcon wrote:

          > Looks good to me. Can you upload this rev of the patch to the JIRA so the QA Bot runs on it?

          Submitted on JIRA. Thanks for the review Todd!

          • Harsh

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          https://reviews.apache.org/r/884/#review805
          -----------------------------------------------------------

          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

          Show
          jiraposter@reviews.apache.org added a comment - On 2011-06-12 02:24:55, Todd Lipcon wrote: > Looks good to me. Can you upload this rev of the patch to the JIRA so the QA Bot runs on it? Submitted on JIRA. Thanks for the review Todd! Harsh ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/884/#review805 ----------------------------------------------------------- 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
          Hide
          jiraposter@reviews.apache.org added a comment -

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          https://reviews.apache.org/r/884/#review817
          -----------------------------------------------------------

          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?

          • Matt

          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

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/884/#review817 ----------------------------------------------------------- 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? Matt 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
          Hide
          jiraposter@reviews.apache.org added a comment -

          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

          Show
          jiraposter@reviews.apache.org added a comment - 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
          Hide
          jiraposter@reviews.apache.org added a comment -

          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?

          Todd Lipcon wrote:

          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." ?

          The public method getSerialization() returns a null if it does not find an acceptor. I think it is fair that a get

          {Serializer,Deserializer}

          () call do the same since they are specific nature calls underneath?

          Or we could revamp the entire set if Exceptions are better to have over null returns and null checks, but it ought to be consistent is what I think.

          • Harsh

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

          Show
          jiraposter@reviews.apache.org added a comment - 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? Todd Lipcon wrote: 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." ? The public method getSerialization() returns a null if it does not find an acceptor. I think it is fair that a get {Serializer,Deserializer} () call do the same since they are specific nature calls underneath? Or we could revamp the entire set if Exceptions are better to have over null returns and null checks, but it ought to be consistent is what I think. Harsh ----------------------------------------------------------- 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
          Hide
          Matt Foley added a comment -

          Moving this discussion back to the Jira to decrease the number and size of emails and jira appends

          It appears to me that getSerializer() should throw an exception on failure to return a usable value, because:

          • The method is only called on objects for which a serializer is expected to exist, so this truly is an "exceptional" condition.
          • The typical caller has no alternate strategy, as far as I know, so it will have to do a check for null then throw an exception anyway.

          The argument for consistency has merit, but this is all Hadoop stuff, so it is okay for us to improve the semantics, if such it would be. Nevertheless, I'm a "0" on this, do what you think best.

          And yes, please, any method that might return null should clearly document that fact in the javadocs.

          Show
          Matt Foley added a comment - Moving this discussion back to the Jira to decrease the number and size of emails and jira appends It appears to me that getSerializer() should throw an exception on failure to return a usable value, because: The method is only called on objects for which a serializer is expected to exist, so this truly is an "exceptional" condition. The typical caller has no alternate strategy, as far as I know, so it will have to do a check for null then throw an exception anyway. The argument for consistency has merit, but this is all Hadoop stuff, so it is okay for us to improve the semantics, if such it would be. Nevertheless, I'm a "0" on this, do what you think best. And yes, please, any method that might return null should clearly document that fact in the javadocs.
          Hide
          Sudharsan Sampath added a comment -

          Just my thoughts...

          To me throwing a specific exception would be better. The checkSerializerSpecs attempts to see if we can initialise a new instance of the serializer/deserializer from the jvm where the job is submitted. How does it guarantee that the libs/jars from which these classes are loaded are available for the jvm that executes the job or vice versa? Even if this methods succeeds isn't there a chance then the original problem might occur due to the libs missing from the actual jvm that executes the job?

          Show
          Sudharsan Sampath added a comment - Just my thoughts... To me throwing a specific exception would be better. The checkSerializerSpecs attempts to see if we can initialise a new instance of the serializer/deserializer from the jvm where the job is submitted. How does it guarantee that the libs/jars from which these classes are loaded are available for the jvm that executes the job or vice versa? Even if this methods succeeds isn't there a chance then the original problem might occur due to the libs missing from the actual jvm that executes the job?
          Hide
          jiraposter@reviews.apache.org added a comment -

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          https://reviews.apache.org/r/884/
          -----------------------------------------------------------

          (Updated 2011-06-16 12:13:34.081758)

          Review request for hadoop-common and Todd Lipcon.

          Changes
          -------

          Throw exceptions (getting rid of nulls). Add appropriate javadocs and fix one checkstyle nit.

          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 (updated)


          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

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/884/ ----------------------------------------------------------- (Updated 2011-06-16 12:13:34.081758) Review request for hadoop-common and Todd Lipcon. Changes ------- Throw exceptions (getting rid of nulls). Add appropriate javadocs and fix one checkstyle nit. 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 (updated) 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
          Hide
          Harsh J added a comment -

          New patch that throws IOExceptions instead.

          Show
          Harsh J added a comment - New patch that throws IOExceptions instead.
          Hide
          Owen O'Malley added a comment -

          Ugh. I hate the messages from review board.

          This is a completely incompatible change. I'd propose adding a new method to SerializationFactory that is about explaining why it wasn't found.

          I need to get back to HADOOP-6685 and move that forward.

          Show
          Owen O'Malley added a comment - Ugh. I hate the messages from review board. This is a completely incompatible change. I'd propose adding a new method to SerializationFactory that is about explaining why it wasn't found. I need to get back to HADOOP-6685 and move that forward.
          Hide
          Harsh J added a comment -

          Owen,

          Yes I noticed it doesn't compile the base after I did an ant clean compile. Was gonna cancel the patch right before your comment went in.

          Looking into the other issue you mention.

          Show
          Harsh J added a comment - Owen, Yes I noticed it doesn't compile the base after I did an ant clean compile . Was gonna cancel the patch right before your comment went in. Looking into the other issue you mention.
          Hide
          Todd Lipcon added a comment -

          This is a completely incompatible change

          The audience for SerializationFactory is private to HDFS and MR, so as long as we have matching patches for those projects, we don't need to consider this a user-facing incompatibility. Right?

          Show
          Todd Lipcon added a comment - This is a completely incompatible change The audience for SerializationFactory is private to HDFS and MR, so as long as we have matching patches for those projects, we don't need to consider this a user-facing incompatibility. Right?
          Hide
          Harsh J added a comment -

          I actually do have two comments of concern w.r.t. front-end & throwing:

          If we are to make code changes throw-y, then it would require that all places that use the said API inside Hadoop add throws IOException to their methods. Upon a quick check, only one single SequenceFile private utility method required this change, and none in MapReduce. But it could've been that the change required subsequent public API changes.

          Since we've never had front end checks for MR serializers before; if people relied on passing strings over to job.xml and have it resolve only on worker machines - the related issue may break this functionality. Guideline-wise, its a wrong way to do it, but I think it still breaks something deep below I think.

          But I think its worth enforcing this check (null handling later or throw above, either way) since it makes stuff like job setup errors quicker to debug and iterate upon.

          Show
          Harsh J added a comment - I actually do have two comments of concern w.r.t. front-end & throwing: If we are to make code changes throw-y , then it would require that all places that use the said API inside Hadoop add throws IOException to their methods. Upon a quick check, only one single SequenceFile private utility method required this change, and none in MapReduce. But it could've been that the change required subsequent public API changes. Since we've never had front end checks for MR serializers before; if people relied on passing strings over to job.xml and have it resolve only on worker machines - the related issue may break this functionality. Guideline-wise, its a wrong way to do it, but I think it still breaks something deep below I think. But I think its worth enforcing this check (null handling later or throw above, either way) since it makes stuff like job setup errors quicker to debug and iterate upon.
          Hide
          Owen O'Malley added a comment -

          The audience for SerializationFactory is private to HDFS and MR.

          The annotations claiming it is limited private is NOT license to change it without severe need. There are other users and this change will break them.

          Show
          Owen O'Malley added a comment - The audience for SerializationFactory is private to HDFS and MR. The annotations claiming it is limited private is NOT license to change it without severe need. There are other users and this change will break them.
          Hide
          Todd Lipcon added a comment -

          Are you OK with replacing the current behavior (throws NPE) with a "return null" instead, like the original patch on this JIRA? Anyone depending on a thrown NPE is just asking for trouble

          Show
          Todd Lipcon added a comment - Are you OK with replacing the current behavior (throws NPE) with a "return null" instead, like the original patch on this JIRA? Anyone depending on a thrown NPE is just asking for trouble
          Hide
          Harsh J added a comment -

          I'm good with returning (and where it applies, checking) nulls. I do not think that breaks anything at all.

          Show
          Harsh J added a comment - I'm good with returning (and where it applies, checking) nulls. I do not think that breaks anything at all.
          Hide
          Harsh J added a comment -

          Is using nulls agreed upon? I'd like to scrap the r3 patch if so, and get the r2 one in instead, so I can do the improvement on the MR side appropriately.

          Show
          Harsh J added a comment - Is using nulls agreed upon? I'd like to scrap the r3 patch if so, and get the r2 one in instead, so I can do the improvement on the MR side appropriately.
          Hide
          Owen O'Malley added a comment -

          Yes, I think the r2 patch is good. You need to write some unit tests to ensure the behavior doesn't regress.

          Show
          Owen O'Malley added a comment - Yes, I think the r2 patch is good. You need to write some unit tests to ensure the behavior doesn't regress.
          Hide
          Harsh J added a comment -

          Revised patch of r2 that adds in a new test as well (per Owen's comment).

          Show
          Harsh J added a comment - Revised patch of r2 that adds in a new test as well (per Owen's comment).
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12483864/HADOOP-7328.r4.diff
          against trunk revision 1139476.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 2 new or modified tests.

          -1 patch. The patch command could not apply the patch.

          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/675//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12483864/HADOOP-7328.r4.diff against trunk revision 1139476. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 2 new or modified tests. -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/675//console This message is automatically generated.
          Hide
          Harsh J added a comment -

          Not sure what went wrong with patch there.

          Re-attaching with common/ prefixes removed.

          Show
          Harsh J added a comment - Not sure what went wrong with patch there. Re-attaching with common/ prefixes removed.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12483867/HADOOP-7328.r4.diff
          against trunk revision 1139476.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 2 new or modified tests.

          -1 patch. The patch command could not apply the patch.

          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/676//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12483867/HADOOP-7328.r4.diff against trunk revision 1139476. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 2 new or modified tests. -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/676//console This message is automatically generated.
          Hide
          Harsh J added a comment -

          Looks like it was a spaces vs. tab issue of some sort that failed the last hunk the last run.

          This should hopefully work now.

          Show
          Harsh J added a comment - Looks like it was a spaces vs. tab issue of some sort that failed the last hunk the last run. This should hopefully work now.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12487008/HADOOP-7328.r5.diff
          against trunk revision 1147971.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 2 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          -1 release audit. The applied patch generated 2 release audit warnings (more than the trunk's current 1 warnings).

          +1 core tests. The patch passed core unit tests.

          +1 system test framework. The patch passed system test framework compile.

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/747//testReport/
          Release audit warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/747//artifact/trunk/patchprocess/patchReleaseAuditProblems.txt
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/747//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/747//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12487008/HADOOP-7328.r5.diff against trunk revision 1147971. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 2 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. -1 release audit. The applied patch generated 2 release audit warnings (more than the trunk's current 1 warnings). +1 core tests. The patch passed core unit tests. +1 system test framework. The patch passed system test framework compile. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/747//testReport/ Release audit warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/747//artifact/trunk/patchprocess/patchReleaseAuditProblems.txt Findbugs warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/747//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/747//console This message is automatically generated.
          Hide
          Harsh J added a comment -

          + Add license to test case. Fixes release audit warning.

          Show
          Harsh J added a comment - + Add license to test case. Fixes release audit warning.
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12487013/HADOOP-7328.r6.diff
          against trunk revision 1147971.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 2 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed core unit tests.

          +1 system test framework. The patch passed system test framework compile.

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/748//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/748//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/748//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - +1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12487013/HADOOP-7328.r6.diff against trunk revision 1147971. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 2 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 system test framework. The patch passed system test framework compile. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/748//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/748//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/748//console This message is automatically generated.
          Hide
          Todd Lipcon added a comment -

          A few small notes:

          • rather than using assertTrue and assertFalse with == checks against null, use assertNull, assertNotNull
          • new tests should use the JUnit 4 API (not extend from TestCase)
          • looks like there is some bad indentation in SerializationFactory.add
          • this JIRA should probably be renamed to indicate its new purpose (ie returning null instead of throwing NPE)
          Show
          Todd Lipcon added a comment - A few small notes: rather than using assertTrue and assertFalse with == checks against null, use assertNull, assertNotNull new tests should use the JUnit 4 API (not extend from TestCase) looks like there is some bad indentation in SerializationFactory.add this JIRA should probably be renamed to indicate its new purpose (ie returning null instead of throwing NPE)
          Hide
          Harsh J added a comment -

          Updating title

          Show
          Harsh J added a comment - Updating title
          Hide
          Harsh J added a comment -

          Patches with Todd's comments fixed. Patches for 0.22, 0.23, Trunk.

          Please review and let me know if further additions are required!

          Will also be updating the dependent JIRA patches for trunk, meanwhile, for this new behavior.

          Show
          Harsh J added a comment - Patches with Todd's comments fixed. Patches for 0.22, 0.23, Trunk. Please review and let me know if further additions are required! Will also be updating the dependent JIRA patches for trunk, meanwhile, for this new behavior.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12492921/0.22-HADOOP-7328.r7.diff
          against trunk revision .

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 2 new or modified tests.

          -1 patch. The patch command could not apply the patch.

          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/132//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12492921/0.22-HADOOP-7328.r7.diff against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 2 new or modified tests. -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/132//console This message is automatically generated.
          Hide
          Harsh J added a comment -

          Reupping trunk patch to get test-patch to go properly. Looks like it picked the wrong name

          Show
          Harsh J added a comment - Reupping trunk patch to get test-patch to go properly. Looks like it picked the wrong name
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12492930/HADOOP-7328.r7.diff
          against trunk revision .

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 2 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed unit tests in hadoop-common-project.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/133//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/133//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-annotations.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/133//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-auth-examples.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/133//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-common.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/133//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-auth.html
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/133//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - +1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12492930/HADOOP-7328.r7.diff against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 2 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed unit tests in hadoop-common-project. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/133//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/133//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-annotations.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/133//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-auth-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/133//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/133//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-auth.html Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/133//console This message is automatically generated.
          Hide
          Todd Lipcon added a comment -

          Latest patch seems good. Does this need to go in at the same time as the dependent patches? Or can it be committed first, with the others later?

          Show
          Todd Lipcon added a comment - Latest patch seems good. Does this need to go in at the same time as the dependent patches? Or can it be committed first, with the others later?
          Hide
          Todd Lipcon added a comment -

          ah, I see the dependent one is also ready to be committed. I committed this to 0.23 and trunk. Can you post one against 0.20-security as well? Seems worth backporting (low risk, common new user mistake)

          Show
          Todd Lipcon added a comment - ah, I see the dependent one is also ready to be committed. I committed this to 0.23 and trunk. Can you post one against 0.20-security as well? Seems worth backporting (low risk, common new user mistake)
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk-Commit #862 (See https://builds.apache.org/job/Hadoop-Common-trunk-Commit/862/)
          HADOOP-7328. When a serializer class is missing, return null, not throw an NPE. Contributed by Harsh J Chouraria.

          todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1167363
          Files :

          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/serializer/SerializationFactory.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/serializer/TestSerializationFactory.java
          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk-Commit #862 (See https://builds.apache.org/job/Hadoop-Common-trunk-Commit/862/ ) HADOOP-7328 . When a serializer class is missing, return null, not throw an NPE. Contributed by Harsh J Chouraria. todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1167363 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/serializer/SerializationFactory.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/serializer/TestSerializationFactory.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk-Commit #939 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/939/)
          HADOOP-7328. When a serializer class is missing, return null, not throw an NPE. Contributed by Harsh J Chouraria.

          todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1167363
          Files :

          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/serializer/SerializationFactory.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/serializer/TestSerializationFactory.java
          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk-Commit #939 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/939/ ) HADOOP-7328 . When a serializer class is missing, return null, not throw an NPE. Contributed by Harsh J Chouraria. todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1167363 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/serializer/SerializationFactory.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/serializer/TestSerializationFactory.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Mapreduce-trunk-Commit #873 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Commit/873/)
          HADOOP-7328. When a serializer class is missing, return null, not throw an NPE. Contributed by Harsh J Chouraria.

          todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1167363
          Files :

          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/serializer/SerializationFactory.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/serializer/TestSerializationFactory.java
          Show
          Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk-Commit #873 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Commit/873/ ) HADOOP-7328 . When a serializer class is missing, return null, not throw an NPE. Contributed by Harsh J Chouraria. todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1167363 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/serializer/SerializationFactory.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/serializer/TestSerializationFactory.java
          Hide
          Harsh J added a comment -

          Yes, I'll post a 0.20-security backport as well.

          Show
          Harsh J added a comment - Yes, I'll post a 0.20-security backport as well.
          Hide
          Harsh J added a comment -

          Ditto patch for 0.20-security.

          • No tab/space issues, so those changes are not present.
          • CommonConfigurationKeys does not seem populated enough in 0.20-security, so that part of the patch isn't present either.

          Tests pass

          
              [junit] Running org.apache.hadoop.io.serializer.TestSerializationFactory
              [junit] Tests run: 1, Failures: 0, Errors: 0, Time elapsed: 0.252 sec
          

          Will upload complementing patch to MAPREDUCE-2584 since it is on the mapred/ side.

          Show
          Harsh J added a comment - Ditto patch for 0.20-security. No tab/space issues, so those changes are not present. CommonConfigurationKeys does not seem populated enough in 0.20-security, so that part of the patch isn't present either. Tests pass [junit] Running org.apache.hadoop.io.serializer.TestSerializationFactory [junit] Tests run: 1, Failures: 0, Errors: 0, Time elapsed: 0.252 sec Will upload complementing patch to MAPREDUCE-2584 since it is on the mapred/ side.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12493899/0.20-security-HADOOP-7328.r7.diff
          against trunk revision .

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 2 new or modified tests.

          -1 patch. The patch command could not apply the patch.

          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/158//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12493899/0.20-security-HADOOP-7328.r7.diff against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 2 new or modified tests. -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/158//console This message is automatically generated.
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk #789 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/789/)
          HADOOP-7328. When a serializer class is missing, return null, not throw an NPE. Contributed by Harsh J Chouraria.

          todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1167363
          Files :

          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/serializer/SerializationFactory.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/serializer/TestSerializationFactory.java
          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk #789 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/789/ ) HADOOP-7328 . When a serializer class is missing, return null, not throw an NPE. Contributed by Harsh J Chouraria. todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1167363 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/serializer/SerializationFactory.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/serializer/TestSerializationFactory.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Mapreduce-trunk #813 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/813/)
          HADOOP-7328. When a serializer class is missing, return null, not throw an NPE. Contributed by Harsh J Chouraria.

          todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1167363
          Files :

          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/serializer/SerializationFactory.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/serializer/TestSerializationFactory.java
          Show
          Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk #813 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/813/ ) HADOOP-7328 . When a serializer class is missing, return null, not throw an NPE. Contributed by Harsh J Chouraria. todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1167363 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/serializer/SerializationFactory.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/serializer/TestSerializationFactory.java
          Hide
          Harsh J added a comment -

          > "Will upload complementing patch to MAPREDUCE-2584 since it is on the mapred/ side."

          Done, complementing patch available on MAPREDUCE-2584 for second-commit.

          Show
          Harsh J added a comment - > "Will upload complementing patch to MAPREDUCE-2584 since it is on the mapred/ side." Done, complementing patch available on MAPREDUCE-2584 for second-commit.
          Hide
          Harsh J added a comment -

          Todd/others,

          This is still pending a commit to branch-0.1.

          Show
          Harsh J added a comment - Todd/others, This is still pending a commit to branch-0.1.
          Hide
          Harsh J added a comment -

          This is also a good candidate for branch-0.23. Will backport in a couple of days if there are no objections.

          Meanwhile, a review/+1 on MAPREDUCE-2584 is also highly welcome as it takes the benefit.

          Show
          Harsh J added a comment - This is also a good candidate for branch-0.23. Will backport in a couple of days if there are no objections. Meanwhile, a review/+1 on MAPREDUCE-2584 is also highly welcome as it takes the benefit.
          Hide
          Harsh J added a comment -

          Huh, this was already committed into 0.23. Unsure why someone marked it wasn't.

          Fix isn't critical for backport, so resolving.

          Show
          Harsh J added a comment - Huh, this was already committed into 0.23. Unsure why someone marked it wasn't. Fix isn't critical for backport, so resolving.
          Hide
          Harsh J added a comment -

          This was committed to 0.23.0.

          Not fixing for 1.x as its not critical.

          MAPREDUCE-2584 depends on this but not vice versa.

          Show
          Harsh J added a comment - This was committed to 0.23.0. Not fixing for 1.x as its not critical. MAPREDUCE-2584 depends on this but not vice versa.

            People

            • Assignee:
              Harsh J
              Reporter:
              Harsh J
            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development