Details
-
Type:
Bug
-
Status: Closed
-
Priority:
Minor
-
Resolution: Fixed
-
Affects Version/s: None
-
Fix Version/s: 2.8.0, 2.7.2, 3.0.0-alpha1
-
Component/s: util
-
Labels:
-
Hadoop Flags:Reviewed
Description
BufferedOutputStream outputStream = new BufferedOutputStream(
new FileOutputStream(outputFile));
...
outputStream.flush();
outputStream.close();
outputStream should be closed in finally block.
Activity
- All
- Comments
- Work Log
- History
- Activity
- Transitions
-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12662721/HADOOP-10365.patch
against trunk revision .
+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 javac. The applied patch does not increase the total number of javac compiler warnings.
+1 javadoc. There were no new javadoc warning messages.
+1 eclipse:eclipse. The patch built with eclipse:eclipse.
+1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) 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.metrics2.impl.TestMetricsSystemImpl
org.apache.hadoop.crypto.key.TestValueQueue
org.apache.hadoop.ipc.TestDecayRpcScheduler
org.apache.hadoop.ipc.TestCallQueueManager
The following test timeouts occurred in hadoop-common-project/hadoop-common:
org.apache.hadoop.http.TestHttpServerLifecycle
+1 contrib tests. The patch passed contrib unit tests.
Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/4508//testReport/
Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/4508//console
This message is automatically generated.
Thanks for your reporting, Sanghyun Yun. As Colin P. McCabe told me on HDFS-6902, it's better to use IOUtils.cleanup(LOG, outputStream) in final block. Do you mind updating a patch?
Tsuyoshi Ozawa, Thanks for your review! I modified code using IOUtils.cleanup() method.
-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12664355/HADOOP-10365.2.patch
against trunk revision .
+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 javac. The applied patch does not increase the total number of javac compiler warnings.
+1 javadoc. There were no new javadoc warning messages.
+1 eclipse:eclipse. The patch built with eclipse:eclipse.
+1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) 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.TestZKFailoverControllerStress
+1 contrib tests. The patch passed contrib unit tests.
Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/4548//testReport/
Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/4548//console
This message is automatically generated.
Please review my patch, Tsuyoshi Ozawa and Ted Yu. 
Sanghyun Yun, thanks for updating a patch. I rethought of the usage of IOUtils - in fact, unpackEntries is only called from FileUtil#unTarUsingJava. FileUtil#unTarUsingJava closes all inputStream in finally block if there are any exceptions in unpackEntries. So, I think we don't need to cleanup the descriptors in unpackEntries. What do you think?
Thanks for your review, Tsuyoshi Ozawa.
I think tis or inputStream in FileUtil#unTarUsingJava is differrent from outputStream in FileUtil#unpackEntries.
outputStream is created newly in FileUtil#unpackEntries. So, need to cleanup.
So, need to cleanup.
Agree. The patch looks mostly good. Minor nits:
1.
+ } catch (IOException e) { + throw e;
These lines are no-op, so they can be removed.
2.
+ outputStream = new BufferedOutputStream(new FileOutputStream(outputFile))
Would you please render the line within 80 characters?
-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12664355/HADOOP-10365.2.patch
against trunk revision 46f6f9d.
+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 javac. The applied patch does not increase the total number of javac compiler warnings.
+1 javadoc. There were no new javadoc warning messages.
+1 eclipse:eclipse. The patch built with eclipse:eclipse.
+1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) 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.TestZKFailoverControllerStress
+1 contrib tests. The patch passed contrib unit tests.
Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/5065//testReport/
Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/5065//console
This message is automatically generated.
Sanghyun Yun Sorry for the delay. Rethinking of using IOUtils - in this case, I found that we should raise IOException to unTarUsingJava and stop the execution immediately when FileOutputStream#close raises IOException. So, could you update the patch based on v1 including comments by Akira?
Thanks for comments, Akira Ajisaka and Tsuyoshi Ozawa.
I agree that FileUtil#unpackEntries() raise the exception.
So, I attached a new patch file.
Sanghyun Yun thanks for your quick updating. Minor nits: could you fix following lines to conform to coding rule?
http://www.oracle.com/technetwork/java/javase/documentation/codeconventions-142311.html#449
if (outputStream != null) outputStream.close();
if (outputStream != null) { outputStream.close(); }
-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12681025/HADOOP-10365.3.patch
against trunk revision 53f64ee.
+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 javac. The applied patch does not increase the total number of javac compiler warnings.
+1 javadoc. There were no new javadoc warning messages.
+1 eclipse:eclipse. The patch built with eclipse:eclipse.
+1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) 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/5067//testReport/
Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/5067//console
This message is automatically generated.
Sorry that breaking the coding rules.
I modified and reattached.
Thanks, Tsuyoshi Ozawa.
-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12681185/HADOOP-10365.4.patch
against trunk revision d7150a1.
+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 javac. The applied patch does not increase the total number of javac compiler warnings.
+1 javadoc. There were no new javadoc warning messages.
+1 eclipse:eclipse. The patch built with eclipse:eclipse.
+1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) 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/5071//testReport/
Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/5071//console
This message is automatically generated.
+1, LGTM.
Sanghyun Yun Sorry for iterative comments - how about using try-with-resources since we only support JDK7+ now. Do you mind updating it?
http://docs.oracle.com/javase/7/docs/technotes/guides/language/try-with-resources.html
Taking this issue as part of Bug Bash. Sanghyun Yun if you are still interested in working on this feel free to assign to yourself.
I have attached a patch using try-with-resources. Tsuyoshi Ozawa review the patch.
 -1 overall |
| Vote | Subsystem | Runtime | Comment |
|---|---|---|---|
| 0 | pre-patch | 15m 6s | 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 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 | javac | 7m 40s | There were no new javac warning messages. |
| +1 | javadoc | 9m 49s | There were no new javadoc warning messages. |
| +1 | release audit | 0m 23s | The applied patch does not increase the total number of release audit warnings. |
| -1 | checkstyle | 1m 7s | The applied patch generated 217 new checkstyle issues (total was 107, now 215). |
| +1 | whitespace | 0m 0s | The patch has no lines that end in whitespace. |
| +1 | install | 1m 39s | mvn install still works. |
| +1 | eclipse:eclipse | 0m 33s | The patch built with eclipse:eclipse. |
| +1 | findbugs | 1m 42s | The patch does not introduce any new Findbugs (version 2.0.3) warnings. |
| +1 | common tests | 23m 11s | Tests passed in hadoop-common. |
| 61m 15s |
| Subsystem | Report/Notes |
|---|---|
| Patch URL | http://issues.apache.org/jira/secure/attachment/12731558/HADOOP-10365.5.patch |
| Optional Tests | javadoc javac unit findbugs checkstyle |
| git revision | trunk / d0e75e6 |
| checkstyle | https://builds.apache.org/job/PreCommit-HADOOP-Build/6555/artifact/patchprocess/diffcheckstylehadoop-common.txt |
| hadoop-common test log | https://builds.apache.org/job/PreCommit-HADOOP-Build/6555/artifact/patchprocess/testrun_hadoop-common.txt |
| Test Results | https://builds.apache.org/job/PreCommit-HADOOP-Build/6555/testReport/ |
| Java | 1.7.0_55 |
| uname | Linux asf903.gq1.ygridcore.net 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux |
| Console output | https://builds.apache.org/job/PreCommit-HADOOP-Build/6555/console |
This message was automatically generated.
Kiran Kumar M R sorry for the delay. Can we remove explicit close of outputStream because we use try-with-resources statement?
+ outputStream.close();
Tsuyoshi Ozawa I have attached a patch after removing outputStream.close(); please review
| -1 overall |
| Vote | Subsystem | Runtime | Comment |
|---|---|---|---|
| 0 | pre-patch | 17m 42s | 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 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 | javac | 8m 13s | There were no new javac warning messages. |
| +1 | javadoc | 10m 18s | There were no new javadoc warning messages. |
| +1 | release audit | 0m 24s | The applied patch does not increase the total number of release audit warnings. |
| +1 | checkstyle | 1m 8s | There were no new checkstyle issues. |
| +1 | whitespace | 0m 0s | The patch has no lines that end in whitespace. |
| +1 | install | 1m 26s | mvn install still works. |
| +1 | eclipse:eclipse | 0m 35s | The patch built with eclipse:eclipse. |
| +1 | findbugs | 1m 59s | The patch does not introduce any new Findbugs (version 3.0.0) warnings. |
| -1 | common tests | 22m 54s | Tests failed in hadoop-common. |
| 64m 43s |
| Reason | Tests |
|---|---|
| Failed unit tests | hadoop.ha.TestZKFailoverController |
| hadoop.ipc.TestCallQueueManager | |
| hadoop.net.TestNetUtils |
| Subsystem | Report/Notes |
|---|---|
| Patch URL | http://issues.apache.org/jira/secure/attachment/12750076/HADOOP-10365.6.patch |
| Optional Tests | javadoc javac unit findbugs checkstyle |
| git revision | trunk / 1c12adb |
| hadoop-common test log | https://builds.apache.org/job/PreCommit-HADOOP-Build/7449/artifact/patchprocess/testrun_hadoop-common.txt |
| Test Results | https://builds.apache.org/job/PreCommit-HADOOP-Build/7449/testReport/ |
| Java | 1.7.0_55 |
| uname | Linux asf905.gq1.ygridcore.net 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux |
| Console output | https://builds.apache.org/job/PreCommit-HADOOP-Build/7449/console |
This message was automatically generated.
Below test failures are unrelated to patch. All are passing locally.
| Reason | Tests |
|---|---|
| Failed unit tests | hadoop.ha.TestZKFailoverController |
| hadoop.ipc.TestCallQueueManager | |
| hadoop.net.TestNetUtils |
Kicking Jenkins again.
+1, pending for Jenkins.
| -1 overall |
| Vote | Subsystem | Runtime | Comment |
|---|---|---|---|
| 0 | pre-patch | 17m 12s | 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 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 | javac | 7m 45s | There were no new javac warning messages. |
| +1 | javadoc | 10m 1s | There were no new javadoc warning messages. |
| +1 | release audit | 0m 23s | The applied patch does not increase the total number of release audit warnings. |
| +1 | checkstyle | 1m 6s | 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 55s | The patch does not introduce any new Findbugs (version 3.0.0) warnings. |
| +1 | common tests | 22m 41s | Tests passed in hadoop-common. |
| 63m 13s |
| Subsystem | Report/Notes |
|---|---|
| Patch URL | http://issues.apache.org/jira/secure/attachment/12753531/HADOOP-10365.6.patch |
| Optional Tests | javadoc javac unit findbugs checkstyle |
| git revision | trunk / 2e251a7 |
| hadoop-common test log | https://builds.apache.org/job/PreCommit-HADOOP-Build/7573/artifact/patchprocess/testrun_hadoop-common.txt |
| Test Results | https://builds.apache.org/job/PreCommit-HADOOP-Build/7573/testReport/ |
| Java | 1.7.0_55 |
| uname | Linux asf906.gq1.ygridcore.net 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux |
| Console output | https://builds.apache.org/job/PreCommit-HADOOP-Build/7573/console |
This message was automatically generated.
| -1 overall |
| Vote | Subsystem | Runtime | Comment |
|---|---|---|---|
| 0 | pre-patch | 16m 45s | 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 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 | javac | 7m 44s | There were no new javac warning messages. |
| +1 | javadoc | 9m 49s | There were no new javadoc warning messages. |
| +1 | release audit | 0m 22s | The applied patch does not increase the total number of release audit warnings. |
| +1 | checkstyle | 1m 6s | There were no new checkstyle issues. |
| +1 | whitespace | 0m 0s | The patch has no lines that end in whitespace. |
| +1 | install | 1m 27s | mvn install still works. |
| +1 | eclipse:eclipse | 0m 33s | The patch built with eclipse:eclipse. |
| +1 | findbugs | 1m 52s | The patch does not introduce any new Findbugs (version 3.0.0) warnings. |
| +1 | common tests | 22m 51s | Tests passed in hadoop-common. |
| 62m 34s |
| Subsystem | Report/Notes |
|---|---|
| Patch URL | http://issues.apache.org/jira/secure/attachment/12753531/HADOOP-10365.6.patch |
| Optional Tests | javadoc javac unit findbugs checkstyle |
| git revision | trunk / 2e251a7 |
| hadoop-common test log | https://builds.apache.org/job/PreCommit-HADOOP-Build/7574/artifact/patchprocess/testrun_hadoop-common.txt |
| Test Results | https://builds.apache.org/job/PreCommit-HADOOP-Build/7574/testReport/ |
| Java | 1.7.0_55 |
| uname | Linux asf901.gq1.ygridcore.net 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux |
| Console output | https://builds.apache.org/job/PreCommit-HADOOP-Build/7574/console |
This message was automatically generated.
Committing this shortly.
Committed this to trunk, branch-2, and branch-2.7. Thanks Kiran Kumar M R and Sanghyun Yun for your contributions!
FAILURE: Integrated in Hadoop-trunk-Commit #8380 (See https://builds.apache.org/job/Hadoop-trunk-Commit/8380/)
HADOOP-10365. BufferedOutputStream in FileUtil#unpackEntries() should be closed in finally block. Contributed by Kiran Kumar M R and Sanghyun Yun. (ozawa: rev dd149adeace8727864371c5a1484c6534f8b450b)
- hadoop-common-project/hadoop-common/CHANGES.txt
- hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileUtil.java
FAILURE: Integrated in Hadoop-Yarn-trunk-Java8 #336 (See https://builds.apache.org/job/Hadoop-Yarn-trunk-Java8/336/)
HADOOP-10365. BufferedOutputStream in FileUtil#unpackEntries() should be closed in finally block. Contributed by Kiran Kumar M R and Sanghyun Yun. (ozawa: rev dd149adeace8727864371c5a1484c6534f8b450b)
- hadoop-common-project/hadoop-common/CHANGES.txt
- hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileUtil.java
FAILURE: Integrated in Hadoop-Yarn-trunk #1063 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/1063/)
HADOOP-10365. BufferedOutputStream in FileUtil#unpackEntries() should be closed in finally block. Contributed by Kiran Kumar M R and Sanghyun Yun. (ozawa: rev dd149adeace8727864371c5a1484c6534f8b450b)
- hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileUtil.java
- hadoop-common-project/hadoop-common/CHANGES.txt
FAILURE: Integrated in Hadoop-Mapreduce-trunk-Java8 #328 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Java8/328/)
HADOOP-10365. BufferedOutputStream in FileUtil#unpackEntries() should be closed in finally block. Contributed by Kiran Kumar M R and Sanghyun Yun. (ozawa: rev dd149adeace8727864371c5a1484c6534f8b450b)
- hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileUtil.java
- hadoop-common-project/hadoop-common/CHANGES.txt
FAILURE: Integrated in Hadoop-Mapreduce-trunk #2277 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/2277/)
HADOOP-10365. BufferedOutputStream in FileUtil#unpackEntries() should be closed in finally block. Contributed by Kiran Kumar M R and Sanghyun Yun. (ozawa: rev dd149adeace8727864371c5a1484c6534f8b450b)
- hadoop-common-project/hadoop-common/CHANGES.txt
- hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileUtil.java
FAILURE: Integrated in Hadoop-Hdfs-trunk-Java8 #320 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Java8/320/)
HADOOP-10365. BufferedOutputStream in FileUtil#unpackEntries() should be closed in finally block. Contributed by Kiran Kumar M R and Sanghyun Yun. (ozawa: rev dd149adeace8727864371c5a1484c6534f8b450b)
- hadoop-common-project/hadoop-common/CHANGES.txt
- hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileUtil.java
FAILURE: Integrated in Hadoop-Hdfs-trunk #2259 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/2259/)
HADOOP-10365. BufferedOutputStream in FileUtil#unpackEntries() should be closed in finally block. Contributed by Kiran Kumar M R and Sanghyun Yun. (ozawa: rev dd149adeace8727864371c5a1484c6534f8b450b)
- hadoop-common-project/hadoop-common/CHANGES.txt
- hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileUtil.java
Thanks Tsuyoshi Ozawa for the review and commit and Sanghyun Yun for the initial patch
I add try-catch-finally block and write outputStream.close() in finally.