Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13304 )
Change subject: [java] Upgrade to log4j 2 ...................................................................... Patch Set 2: (3 comments) http://gerrit.cloudera.org:8080/#/c/13304/1/java/kudu-test-utils/src/main/java/org/apache/kudu/test/CapturingToFileLogAppender.java File java/kudu-test-utils/src/main/java/org/apache/kudu/test/CapturingToFileLogAppender.java: http://gerrit.cloudera.org:8080/#/c/13304/1/java/kudu-test-utils/src/main/java/org/apache/kudu/test/CapturingToFileLogAppender.java@74 PS1, Line 74: try (OutputStream os = createOutputStream(useGzip)) { > Done Missed this. http://gerrit.cloudera.org:8080/#/c/13304/2/java/kudu-test-utils/src/main/java/org/apache/kudu/test/CapturingToFileLogAppender.java File java/kudu-test-utils/src/main/java/org/apache/kudu/test/CapturingToFileLogAppender.java: http://gerrit.cloudera.org:8080/#/c/13304/2/java/kudu-test-utils/src/main/java/org/apache/kudu/test/CapturingToFileLogAppender.java@77 PS2, Line 77: outputFileWriter = new BufferedWriter(new OutputStreamWriter(os, UTF_8)); I guess there's also an issue with the old code if the BufferedWriter constructor throws: who closes the OutputStreamWriter? Or is it sufficient to close 'os'? http://gerrit.cloudera.org:8080/#/c/13304/2/java/kudu-test-utils/src/main/java/org/apache/kudu/test/CapturingToFileLogAppender.java@87 PS2, Line 87: os = new GZIPOutputStream(os); If this throws, where do we close 'os' (i.e. the FileOutputStream created on L85)? That's what the old code was trying to do with the inner catch. I don't think the new try-with-resources in the constructor does that, because if createOutputStream throws, 'os' will not have been assigned. -- To view, visit http://gerrit.cloudera.org:8080/13304 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic4e14a3a25c36d5f47d07f06c1c0e68dad42bc66 Gerrit-Change-Number: 13304 Gerrit-PatchSet: 2 Gerrit-Owner: Grant Henke <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Mon, 13 May 2019 18:10:59 +0000 Gerrit-HasComments: Yes
