-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12484872/MAPREDUCE-2631.patch
against trunk revision 1140942.

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

+1 tests included. The patch appears to include 6 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 3 release audit warnings (more than the trunk's current 2 warnings).

-1 core tests. The patch failed these core unit tests:
org.apache.hadoop.cli.TestMRCLI
org.apache.hadoop.fs.TestFileSystem

-1 contrib tests. The patch failed contrib unit tests.

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

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

This message is automatically generated.

Resolved the release audit comments.

-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12485401/MAPREDUCE-2631.1.patch
against trunk revision 1143252.

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

+1 tests included. The patch appears to include 6 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 failed these core unit tests:
org.apache.hadoop.cli.TestMRCLI
org.apache.hadoop.fs.TestFileSystem

-1 contrib tests. The patch failed contrib unit tests.

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

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

This message is automatically generated.

The testcase failures are not related to this patch.

• There is at least one hard tab character introduced - please use 2 space indentation.
• please write new unit test files using Junit 4 style instead of inheriting from TestCase

otherwise looks good. thanks

Hi Todd, thanks for pointing it out.
Fixed 1 additional issue, by adding the null check ,avoiding the closing of the stream twice.

-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12485732/MAPREDUCE-2631.2.patch
against trunk revision 1144097.

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

+1 tests included. The patch appears to include 6 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 failed these core unit tests:
org.apache.hadoop.cli.TestMRCLI
org.apache.hadoop.fs.TestFileSystem

-1 contrib tests. The patch failed contrib unit tests.

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

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

This message is automatically generated.

The testcase failures are not related to this patch.
This fix is inline with the stream closure pattern in HDFS-1753.

Sorry, the patch has gone stale. Can you please rebase? Thanks.

Rebased the patch to the latest version.

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

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

+1 tests included. The patch appears to include 6 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 appears to introduce 14 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:

org.apache.hadoop.yarn.server.nodemanager.TestNMAuditLogger

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

Test results: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/638//testReport/
Findbugs warnings: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/638//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-mapreduce-client-common.html
Findbugs warnings: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/638//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-mapreduce-client-hs.html
Findbugs warnings: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/638//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-mapreduce-client-core.html
Findbugs warnings: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/638//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-mapreduce-client-app.html
Findbugs warnings: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/638//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-mapreduce-client-shuffle.html
Findbugs warnings: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/638//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-mapreduce-client-jobclient.html
Findbugs warnings: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/638//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-yarn-api.html
Findbugs warnings: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/638//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-yarn-server-nodemanager.html
Findbugs warnings: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/638//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-yarn-server-resourcemanager.html
Findbugs warnings: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/638//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-yarn-server-common.html
Findbugs warnings: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/638//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-yarn-common.html
Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/638//console

This message is automatically generated.

The and findbugs testcase failures are not related to this patch.Thanks.

-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12493553/MAPREDUCE-2631.3.patch
against trunk revision 9a37247.

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

Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5222//console

This message is automatically generated.

Cancelling patch as it no longer applies.

Rebasing patch against trunk. Patch still looks valid.

Activity done as part of bugbash0515.

Submitting the change to kick jenkins.
Ravi Teja Ch N V pls reassign if you are interested still.

Thanks Sunil G for the updated patch.

https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5670/console

-1 overall

| Vote |           Subsystem |  Runtime   | Comment
============================================================================
|   0  |          pre-patch  |  14m 46s   | Pre-patch trunk compilation is 
|      |                     |            | healthy.
|  +1  |            @author  |  0m 0s     | The patch does not contain any 
|      |                     |            | @author tags.
|  +1  |     tests included  |  0m 0s     | The patch appears to include 2 new 
|      |                     |            | or modified test files.
|  +1  |              javac  |  7m 39s    | There were no new javac warning 
|      |                     |            | messages.
|  +1  |            javadoc  |  9m 51s    | There were no new javadoc warning 
|      |                     |            | messages.
|  +1  |      release audit  |  0m 21s    | The applied patch does not increase 
|      |                     |            | the total number of release audit
|      |                     |            | warnings.
|  +1  |         checkstyle  |  1m 16s    | There were no new checkstyle 
|      |                     |            | issues.
|  +1  |         whitespace  |  0m 0s     | The patch has no lines that end in 
|      |                     |            | whitespace.
|  +1  |            install  |  1m 33s    | mvn install still works. 
|  +1  |    eclipse:eclipse  |  0m 32s    | The patch built with 
|      |                     |            | eclipse:eclipse.
|  +1  |           findbugs  |  1m 56s    | The patch does not introduce any 
|      |                     |            | new Findbugs (version 2.0.3)
|      |                     |            | warnings.
|  +1  |    mapreduce tests  |  1m 36s    | Tests passed in 
|      |                     |            | hadoop-mapreduce-client-core.
|  -1  |    mapreduce tests  |  97m 32s   | Tests failed in 
|      |                     |            | hadoop-mapreduce-client-jobclient.
|      |                     |  137m 6s   | 


             Reason | Tests
 Failed unit tests  |  hadoop.mapreduce.security.ssl.TestEncryptedShuffle 
                    |  hadoop.mapreduce.security.TestBinaryTokenFile 
                    |  hadoop.mapreduce.TestMapReduceLazyOutput 
                    |  hadoop.mapred.pipes.TestPipeApplication 
                    |  hadoop.mapreduce.TestMRJobClient 

Can you have a look into these failed tests?

-1 overall

This message was automatically generated.

Thanks for the patch Sunil. In BinaryProtocol.close(), if out.close() throws an exception, file will never be set to null right?
Uploading a new patch which does this:

    public void close() throws IOException {
      try {
        flush();
      } finally {
        IOUtils.closeStream(file);
        file = null;
        IOUtils.closeStream(file);
        out = null;
      }
    }

Similarly for IFileOutputStream . Could you please review it?

-1 overall

This message was automatically generated.

Kicking jenkins again.

Uploading same patch again to check Test failures.

-1 overall

This message was automatically generated.

-1 overall

This message was automatically generated.

-1 overall

This message was automatically generated.

Thanks Sunil G for the patch. Can you have a look into this failure whether it is related to the patch?

Tests run: 5, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 5.184 sec <<< FAILURE! - in org.apache.hadoop.mapred.pipes.TestPipeApplication
testApplication(org.apache.hadoop.mapred.pipes.TestPipeApplication)  Time elapsed: 1.994 sec  <<< ERROR!
java.lang.NullPointerException: null
	at org.apache.hadoop.mapred.pipes.BinaryProtocol$TeeOutputStream.write(BinaryProtocol.java:204)
	at java.io.BufferedOutputStream.flushBuffer(BufferedOutputStream.java:82)
	at java.io.BufferedOutputStream.flush(BufferedOutputStream.java:140)
	at java.io.DataOutputStream.flush(DataOutputStream.java:123)
	at org.apache.hadoop.mapred.pipes.BinaryProtocol.flush(BinaryProtocol.java:349)
	at org.apache.hadoop.mapred.pipes.Application.abort(Application.java:217)
	at org.apache.hadoop.mapred.pipes.TestPipeApplication.testApplication(TestPipeApplication.java:224)

Thank you Devaraj K
This test failure is due to the patch. I will fix and will upload a new version.

Cancelling patch to fix the test failure.

Uploading patch after correcting test failures.

-1 overall

This message was automatically generated.

Hi Devaraj K
Updating a patch after correcting few warnings.

-1 overall

This message was automatically generated.

Hi Devaraj K
Could you please review the latest patch.

Hi Sunil G,
I am fine with the approach, can you rebase the patch as its not getting applied....

Rebasing patch.

Thanks for the patch Sunil G but i have some concerns with the approach, i feel its not required to have setting to null and check for whether its closed because it would be better to throw exception when someone tries to write on a the stream which is already closed rather than silently return without exception. Also there is not much optimization when we set to the file and stream to null on close.

As per the offline discussion with Naganarasimha Garla, file or out need not have to be set null. So we can avoid isClosed. We will get an IOException since the streams are already closed. I think in such cases, caller can get exception rather than suppressing the same.

we need to remove out = null; in IFileOutputStream too

Yes. Updating patch for same.

-1 overall

This message was automatically generated.

Thanks for the contributions Sunil G and review from Ravi Prakash, Todd Lipcon.
Committed it trunk, branch-2 and branch-2.8

SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #10714 (See https://builds.apache.org/job/Hadoop-trunk-Commit/10714/)
MAPREDUCE-2631. Potential resource leaks in (naganarasimha_gr: rev 28660f51af161a9fa301523d96a6f8ae4ebd6edd)

• (edit) hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapred/pipes/BinaryProtocol.java
• (edit) hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-jobclient/src/test/java/org/apache/hadoop/mapred/TestIFileStreams.java
• (edit) hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapred/IFileOutputStream.java

Thanks Naganarasimha Garla for the commit and thanks Ravi Teja for the initial patch. Thanks Ravi Prakash and Todd for the reviews.