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

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

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.

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

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!

Re-upping same old patch to trigger jenkins.

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

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

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

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

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

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

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