Hadoop Common
  1. Hadoop Common
  2. HADOOP-8531

SequenceFile Writer can throw out a better error if a serializer or deserializer isn't available

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Trivial Trivial
    • Resolution: Fixed
    • Affects Version/s: 2.0.0-alpha
    • Fix Version/s: 2.0.2-alpha
    • Component/s: io
    • Labels:
    • Hadoop Flags:
      Reviewed

      Description

      Currently, if the provided Key/Value class lacks a proper serializer in the loaded config for the SequenceFile.Writer, we get an NPE as the null return goes unchecked.

      Hence we get:

      java.lang.NullPointerException
      	at org.apache.hadoop.io.SequenceFile$Writer.init(SequenceFile.java:1163)
      	at org.apache.hadoop.io.SequenceFile$Writer.<init>(SequenceFile.java:1079)
      	at org.apache.hadoop.io.SequenceFile$RecordCompressWriter.<init>(SequenceFile.java:1331)
      	at org.apache.hadoop.io.SequenceFile.createWriter(SequenceFile.java:271)
      

      We can provide a better message + exception in such cases. This is slightly related to MAPREDUCE-2584.

      1. HADOOP-8531-2.patch
        7 kB
        Harsh J
      2. HADOOP-8531-2.patch
        7 kB
        Harsh J
      3. HADOOP-8531-1.patch
        3 kB
        madhukara phatak
      4. HADOOP-8531.patch
        3 kB
        madhukara phatak

        Issue Links

          Activity

          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12533619/HADOOP-8531.patch
          against trunk revision .

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

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

          -1 javac. The applied patch generated 2056 javac compiler warnings (more than the trunk's current 2054 warnings).

          -1 javadoc. The javadoc tool appears to have generated 2 warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          +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 unit tests in hadoop-common-project/hadoop-common:

          org.apache.hadoop.fs.viewfs.TestViewFsTrash

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

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/1143//testReport/
          Javac warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/1143//artifact/trunk/trunk/patchprocess/diffJavacWarnings.txt
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/1143//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/12533619/HADOOP-8531.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 1 new or modified test files. -1 javac. The applied patch generated 2056 javac compiler warnings (more than the trunk's current 2054 warnings). -1 javadoc. The javadoc tool appears to have generated 2 warning messages. +1 eclipse:eclipse. The patch built with eclipse:eclipse. +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 unit tests in hadoop-common-project/hadoop-common: org.apache.hadoop.fs.viewfs.TestViewFsTrash +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/1143//testReport/ Javac warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/1143//artifact/trunk/trunk/patchprocess/diffJavacWarnings.txt Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/1143//console This message is automatically generated.
          Hide
          Harsh J added a comment -

          Hi,

          Thanks for the patch. Functionally it looks good. However, please address these style and message nits that needs to be followed to ensure uniform code look across Hadoop code

          In general we follow Sun's Code style conventions, with 2 spaces for indentation.

          if(this.keySerializer == null) {

          Please place a space after if.

          "Not able to get serializer for value class "+valClass.getCanonicalName()

          Please place a space on either sides of +. Additionally, in this message and in other messages such as this, it would be better to borrow the style I've used at MAPREDUCE-2584, that also prompts the user to check their core-site.xml configuration properties to load proper classes to make this operation successful.

          fs,conf, path,

          Please space out the commas properly between fs and conf, as done with the other params. Same applies to other places you would notice this in your patch.

          fail("should throw IOException");

          The failing message can also be more clear on why an IOException is expected here (i.e. purpose, being missing serializers or deserializers).

          Can you also test if deserializer-fetching needs the same work and edit the patch appropriately?

          Thanks again!

          P.s. Please do keep in mind the code conventions when submitting patches as it helps with speedier acceptance

          Show
          Harsh J added a comment - Hi, Thanks for the patch. Functionally it looks good. However, please address these style and message nits that needs to be followed to ensure uniform code look across Hadoop code In general we follow Sun's Code style conventions, with 2 spaces for indentation. if ( this .keySerializer == null ) { Please place a space after if . "Not able to get serializer for value class " +valClass.getCanonicalName() Please place a space on either sides of + . Additionally, in this message and in other messages such as this, it would be better to borrow the style I've used at MAPREDUCE-2584 , that also prompts the user to check their core-site.xml configuration properties to load proper classes to make this operation successful. fs,conf, path, Please space out the commas properly between fs and conf , as done with the other params. Same applies to other places you would notice this in your patch. fail( "should throw IOException" ); The failing message can also be more clear on why an IOException is expected here (i.e. purpose, being missing serializers or deserializers). Can you also test if deserializer-fetching needs the same work and edit the patch appropriately? Thanks again! P.s. Please do keep in mind the code conventions when submitting patches as it helps with speedier acceptance
          Hide
          madhukara phatak added a comment -

          Hi Harsha,
          Thanks for reviewing patch. I have updated the patch which fixes style issues and error messages. Deserializer code already has null checks , so I think there is no need of any change in them.

          Show
          madhukara phatak added a comment - Hi Harsha, Thanks for reviewing patch. I have updated the patch which fixes style issues and error messages. Deserializer code already has null checks , so I think there is no need of any change in them.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12534147/HADOOP-8531-1.patch
          against trunk revision .

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

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

          -1 javac. The applied patch generated 2072 javac compiler warnings (more than the trunk's current 2070 warnings).

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

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          +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/hadoop-common.

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

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/1159//testReport/
          Javac warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/1159//artifact/trunk/trunk/patchprocess/diffJavacWarnings.txt
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/1159//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/12534147/HADOOP-8531-1.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 1 new or modified test files. -1 javac. The applied patch generated 2072 javac compiler warnings (more than the trunk's current 2070 warnings). +1 javadoc. The javadoc tool did not generate any warning messages. +1 eclipse:eclipse. The patch built with eclipse:eclipse. +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/hadoop-common. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/1159//testReport/ Javac warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/1159//artifact/trunk/trunk/patchprocess/diffJavacWarnings.txt Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/1159//console This message is automatically generated.
          Hide
          Harsh J added a comment -

          Your patch looks good so I went ahead and additionally added the error message enhancements (to note configs) and also added in checks for the deserializer (they were not present, unsure what you meant when you said they were - what am I missing?)

          This new patch uses proper writer/reader constructors for sequence files as well, instead of the deprecated constructors that existed in the patch previously.

          Running org.apache.hadoop.io.TestSequenceFile
          Tests run: 8, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 13.4 sec
          

          Added tests pass, as shown above.

          I'll commit this in after jenkins gives its +1. Thanks Madhukara!

          Show
          Harsh J added a comment - Your patch looks good so I went ahead and additionally added the error message enhancements (to note configs) and also added in checks for the deserializer (they were not present, unsure what you meant when you said they were - what am I missing?) This new patch uses proper writer/reader constructors for sequence files as well, instead of the deprecated constructors that existed in the patch previously. Running org.apache.hadoop.io.TestSequenceFile Tests run: 8, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 13.4 sec Added tests pass, as shown above. I'll commit this in after jenkins gives its +1. Thanks Madhukara!
          Hide
          Harsh J added a comment -

          Re-upping same old patch to trigger jenkins.

          Show
          Harsh J added a comment - Re-upping same old patch to trigger jenkins.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12536561/HADOOP-8531-2.patch
          against trunk revision .

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

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

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

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

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          +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 unit tests in hadoop-common-project/hadoop-common:

          org.apache.hadoop.ha.TestZKFailoverController
          org.apache.hadoop.io.file.tfile.TestTFileByteArrays
          org.apache.hadoop.io.file.tfile.TestTFileJClassComparatorByteArrays

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

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/1197//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/1197//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/12536561/HADOOP-8531-2.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 1 new or modified test files. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 javadoc. The javadoc tool did not generate any warning messages. +1 eclipse:eclipse. The patch built with eclipse:eclipse. +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 unit tests in hadoop-common-project/hadoop-common: org.apache.hadoop.ha.TestZKFailoverController org.apache.hadoop.io.file.tfile.TestTFileByteArrays org.apache.hadoop.io.file.tfile.TestTFileJClassComparatorByteArrays +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/1197//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/1197//console This message is automatically generated.
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk-Commit #2477 (See https://builds.apache.org/job/Hadoop-Common-trunk-Commit/2477/)
          HADOOP-8531. SequenceFile Writer can throw out a better error if a serializer or deserializer isn't available. Contributed by Madhukara Phatak. (harsh) (Revision 1361933)

          Result = SUCCESS
          harsh : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1361933
          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/SequenceFile.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/TestSequenceFile.java
          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk-Commit #2477 (See https://builds.apache.org/job/Hadoop-Common-trunk-Commit/2477/ ) HADOOP-8531 . SequenceFile Writer can throw out a better error if a serializer or deserializer isn't available. Contributed by Madhukara Phatak. (harsh) (Revision 1361933) Result = SUCCESS harsh : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1361933 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/SequenceFile.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/TestSequenceFile.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk-Commit #2542 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/2542/)
          HADOOP-8531. SequenceFile Writer can throw out a better error if a serializer or deserializer isn't available. Contributed by Madhukara Phatak. (harsh) (Revision 1361933)

          Result = SUCCESS
          harsh : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1361933
          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/SequenceFile.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/TestSequenceFile.java
          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk-Commit #2542 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/2542/ ) HADOOP-8531 . SequenceFile Writer can throw out a better error if a serializer or deserializer isn't available. Contributed by Madhukara Phatak. (harsh) (Revision 1361933) Result = SUCCESS harsh : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1361933 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/SequenceFile.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/TestSequenceFile.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Mapreduce-trunk-Commit #2497 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Commit/2497/)
          HADOOP-8531. SequenceFile Writer can throw out a better error if a serializer or deserializer isn't available. Contributed by Madhukara Phatak. (harsh) (Revision 1361933)

          Result = FAILURE
          harsh : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1361933
          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/SequenceFile.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/TestSequenceFile.java
          Show
          Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk-Commit #2497 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Commit/2497/ ) HADOOP-8531 . SequenceFile Writer can throw out a better error if a serializer or deserializer isn't available. Contributed by Madhukara Phatak. (harsh) (Revision 1361933) Result = FAILURE harsh : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1361933 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/SequenceFile.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/TestSequenceFile.java
          Hide
          Harsh J added a comment -

          Committed to branch-2 and trunk. Thanks for the patch Madhukara!

          Show
          Harsh J added a comment - Committed to branch-2 and trunk. Thanks for the patch Madhukara!
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk #1105 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1105/)
          HADOOP-8531. SequenceFile Writer can throw out a better error if a serializer or deserializer isn't available. Contributed by Madhukara Phatak. (harsh) (Revision 1361933)

          Result = FAILURE
          harsh : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1361933
          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/SequenceFile.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/TestSequenceFile.java
          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk #1105 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1105/ ) HADOOP-8531 . SequenceFile Writer can throw out a better error if a serializer or deserializer isn't available. Contributed by Madhukara Phatak. (harsh) (Revision 1361933) Result = FAILURE harsh : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1361933 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/SequenceFile.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/TestSequenceFile.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Mapreduce-trunk #1138 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1138/)
          HADOOP-8531. SequenceFile Writer can throw out a better error if a serializer or deserializer isn't available. Contributed by Madhukara Phatak. (harsh) (Revision 1361933)

          Result = FAILURE
          harsh : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1361933
          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/SequenceFile.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/TestSequenceFile.java
          Show
          Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk #1138 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1138/ ) HADOOP-8531 . SequenceFile Writer can throw out a better error if a serializer or deserializer isn't available. Contributed by Madhukara Phatak. (harsh) (Revision 1361933) Result = FAILURE harsh : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1361933 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/SequenceFile.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/TestSequenceFile.java

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development