This is an automated email from the ASF dual-hosted git repository. adar pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/kudu.git
commit 37e53bf1d99e2ff44f0288ed65321f9004736813 Author: Adar Dembo <[email protected]> AuthorDate: Thu May 23 16:57:20 2019 -0700 java: fix CapturingToFileLogAppender It was broken in commit 0114ce1d0: the try-with-resources in the constructor closed the OutputStream out from under the BufferedWriter. I added a regression test for this. Change-Id: I7fa14df5cd4025236ec398acf4973520d9dea082 Reviewed-on: http://gerrit.cloudera.org:8080/13418 Reviewed-by: Hao Hao <[email protected]> Tested-by: Adar Dembo <[email protected]> --- .../org/apache/kudu/test/CapturingLogAppender.java | 12 +- .../kudu/test/CapturingToFileLogAppender.java | 29 ++++- .../kudu/test/TestCapturingToFileLogAppender.java | 141 +++++++++++++++++++++ 3 files changed, 174 insertions(+), 8 deletions(-) diff --git a/java/kudu-test-utils/src/main/java/org/apache/kudu/test/CapturingLogAppender.java b/java/kudu-test-utils/src/main/java/org/apache/kudu/test/CapturingLogAppender.java index 4bf1d88..932e14a 100644 --- a/java/kudu-test-utils/src/main/java/org/apache/kudu/test/CapturingLogAppender.java +++ b/java/kudu-test-utils/src/main/java/org/apache/kudu/test/CapturingLogAppender.java @@ -18,6 +18,7 @@ package org.apache.kudu.test; import java.io.Closeable; import java.io.IOException; +import java.util.Random; import com.google.common.base.Throwables; import org.apache.logging.log4j.core.LogEvent; @@ -44,8 +45,15 @@ public class CapturingLogAppender extends AbstractAppender { private StringBuilder appended = new StringBuilder(); public CapturingLogAppender() { - super("CapturingLogAppender", /* filter */ null, LAYOUT, - /* ignoreExceptions */ true, Property.EMPTY_ARRAY); + // Appender name must be unique so that attaching/detaching works correctly + // when multiple capturing appenders are used recursively. + super(String.format("CapturingToFileLogAppender-%d", new Random().nextInt()), + /* filter */ null, LAYOUT, /* ignoreExceptions */ true, Property.EMPTY_ARRAY); + + // If we don't call start(), we get an ugly log error: + // + // ERROR Attempted to append to non-started appender CapturingToFileLogAppender + start(); } @Override diff --git a/java/kudu-test-utils/src/main/java/org/apache/kudu/test/CapturingToFileLogAppender.java b/java/kudu-test-utils/src/main/java/org/apache/kudu/test/CapturingToFileLogAppender.java index c0009a8..7a63dcc 100644 --- a/java/kudu-test-utils/src/main/java/org/apache/kudu/test/CapturingToFileLogAppender.java +++ b/java/kudu-test-utils/src/main/java/org/apache/kudu/test/CapturingToFileLogAppender.java @@ -24,6 +24,7 @@ import java.io.IOException; import java.io.OutputStream; import java.io.OutputStreamWriter; import java.io.Writer; +import java.util.Random; import java.util.zip.GZIPOutputStream; import com.google.common.base.Throwables; @@ -69,17 +70,33 @@ public class CapturingToFileLogAppender extends AbstractAppender implements Auto * @param useGzip whether to gzip-compress messages when appended */ public CapturingToFileLogAppender(boolean useGzip) throws IOException { - super("CapturingToFileLogAppender", /* filter */ null, LAYOUT, - /* ignoreExceptions */ true, Property.EMPTY_ARRAY); + // Appender name must be unique so that attaching/detaching works correctly + // when multiple capturing appenders are used recursively. + super(String.format("CapturingToFileLogAppender-%d", new Random().nextInt()), + /* filter */ null, LAYOUT, /* ignoreExceptions */ true, Property.EMPTY_ARRAY); + outputFile = File.createTempFile("captured_output", ".txt.gz"); - try (OutputStream os = createOutputStream(useGzip)) { - // As per the recommendation in OutputStreamWriter's Javadoc, we wrap in a - // BufferedWriter to buffer up character conversions. - outputFileWriter = new BufferedWriter(new OutputStreamWriter(os, UTF_8)); + try { + OutputStream os = createOutputStream(useGzip); + try { + // As per the recommendation in OutputStreamWriter's Javadoc, we wrap in + // a BufferedWriter to buffer up character conversions. + outputFileWriter = new BufferedWriter(new OutputStreamWriter(os, UTF_8)); + } catch (Throwable t) { + os.close(); + } } catch (Throwable t) { outputFile.delete(); throw t; } + + // If we don't call start(), we get an ugly log error: + // + // ERROR Attempted to append to non-started appender CapturingToFileLogAppender + // + // It doesn't throw anything so there's no reason to include it in the above + // try/catch monstrosity. + start(); } private OutputStream createOutputStream(boolean useGzip) throws IOException { diff --git a/java/kudu-test-utils/src/test/java/org/apache/kudu/test/TestCapturingToFileLogAppender.java b/java/kudu-test-utils/src/test/java/org/apache/kudu/test/TestCapturingToFileLogAppender.java new file mode 100644 index 0000000..042bcaa --- /dev/null +++ b/java/kudu-test-utils/src/test/java/org/apache/kudu/test/TestCapturingToFileLogAppender.java @@ -0,0 +1,141 @@ +/** + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. See accompanying LICENSE file. + */ +package org.apache.kudu.test; + +import static java.nio.charset.StandardCharsets.UTF_8; + +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertTrue; + +import com.google.common.base.Joiner; + +import org.apache.kudu.test.junit.RetryRule; +import org.junit.Rule; +import org.junit.Test; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.io.BufferedReader; +import java.io.Closeable; +import java.io.File; +import java.io.FileInputStream; +import java.io.InputStream; +import java.io.InputStreamReader; +import java.io.IOException; +import java.io.Reader; +import java.util.ArrayList; +import java.util.List; +import java.util.zip.GZIPInputStream; + +public class TestCapturingToFileLogAppender { + private static final Logger LOG = + LoggerFactory.getLogger(TestCapturingToFileLogAppender.class); + + private static final String MAGIC_STRING = "hello world!"; + + @Rule + public RetryRule retryRule = new RetryRule(); + + private String readAllFromBufferedReader(BufferedReader br) throws IOException { + List<String> output = new ArrayList<>(); + for (String line = br.readLine(); line != null; line = br.readLine()) { + output.add(line); + } + return Joiner.on("\n").join(output); + } + + private String readAllFromFile(File fileName) throws IOException { + try (InputStream fis = new FileInputStream(fileName); + Reader isr = new InputStreamReader(fis, UTF_8); + BufferedReader br = new BufferedReader(isr)) { + return readAllFromBufferedReader(br); + } + } + + private String readAllFromGzippedFile(File fileName) throws IOException { + try (InputStream fis = new FileInputStream(fileName); + InputStream gzis = new GZIPInputStream(fis); + Reader isr = new InputStreamReader(gzis, UTF_8); + BufferedReader br = new BufferedReader(isr)) { + return readAllFromBufferedReader(br); + } + } + + @Test + public void testLog() throws IOException { + File outputFile; + try (CapturingToFileLogAppender capturer = + new CapturingToFileLogAppender(/*useGzip=*/ false)) { + outputFile = capturer.getOutputFile(); + assertTrue(outputFile.exists()); + + // Log a magic string and flush the output file. + try (Closeable c = capturer.attach()) { + LOG.info(MAGIC_STRING); + } + capturer.finish(); + + // Read the magic string out of the output file. + String captured = readAllFromFile(outputFile); + assertNotNull(captured); + assertTrue(captured.contains(MAGIC_STRING)); + } + assertFalse(outputFile.exists()); + } + + @Test + public void testLogGzipped() throws IOException { + File outputFile; + try (CapturingToFileLogAppender capturer = + new CapturingToFileLogAppender(/*useGzip=*/ true)) { + outputFile = capturer.getOutputFile(); + assertTrue(outputFile.exists()); + + // Log a magic string and flush the output file. + try (Closeable c = capturer.attach()) { + LOG.info(MAGIC_STRING); + } + capturer.finish(); + + // Read the magic string out of the output file. + String captured = readAllFromGzippedFile(outputFile); + assertNotNull(captured); + assertTrue(captured.contains(MAGIC_STRING)); + } + assertFalse(outputFile.exists()); + } + + @Test + public void testLogException() throws IOException { + File outputFile; + try (CapturingToFileLogAppender capturer = + new CapturingToFileLogAppender(/*useGzip=*/ false)) { + outputFile = capturer.getOutputFile(); + assertTrue(outputFile.exists()); + + // Log a magic string and flush the output file. + try (Closeable c = capturer.attach()) { + LOG.error("Saw exception", new Exception(MAGIC_STRING)); + } + capturer.finish(); + + // Read the magic string out of the output file. + String captured = readAllFromFile(outputFile); + assertNotNull(captured); + assertTrue(captured.contains("java.lang.Exception: " + MAGIC_STRING)); + } + assertFalse(outputFile.exists()); + } +}
