From a11c230236036317a6c12deeca401e3ae8dce698 Mon Sep 17 00:00:00 2001 From: Andrew Wang Date: Tue, 1 Aug 2017 15:13:29 -0700 Subject: [PATCH] HADOOP-14701. Configuration can log misleading warnings about an attempt to override final parameter. Contributed by Andrew Sherman. --- .../org/apache/hadoop/conf/Configuration.java | 21 ++- .../apache/hadoop/conf/TestConfiguration.java | 175 ++++++++++++++++++ 2 files changed, 193 insertions(+), 3 deletions(-) diff --git a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/conf/Configuration.java b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/conf/Configuration.java index de52fbb00e..e26d3a8c04 100644 --- a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/conf/Configuration.java +++ b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/conf/Configuration.java @@ -2911,9 +2911,12 @@ public class Configuration implements Iterable>, if(source != null) { updatingResource.put(attr, source); } - } else if (!value.equals(properties.getProperty(attr))) { - LOG.warn(name+":an attempt to override final parameter: "+attr - +"; Ignoring."); + } else { + // This is a final parameter so check for overrides. + checkForOverride(this.properties, name, attr, value); + if (this.properties != properties) { + checkForOverride(properties, name, attr, value); + } } } if (finalParameter && attr != null) { @@ -2921,6 +2924,18 @@ public class Configuration implements Iterable>, } } + /** + * Print a warning if a property with a given name already exists with a + * different value + */ + private void checkForOverride(Properties properties, String name, String attr, String value) { + String propertyValue = properties.getProperty(attr); + if (propertyValue != null && !propertyValue.equals(value)) { + LOG.warn(name + ":an attempt to override final parameter: " + attr + + "; Ignoring."); + } + } + /** * Write out the non-default properties in this configuration to the given * {@link OutputStream} using UTF-8 encoding. diff --git a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/conf/TestConfiguration.java b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/conf/TestConfiguration.java index 5ced541af3..2af61c069c 100644 --- a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/conf/TestConfiguration.java +++ b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/conf/TestConfiguration.java @@ -36,6 +36,7 @@ import java.util.Arrays; import java.util.Collection; import java.util.HashMap; import java.util.HashSet; +import java.util.List; import java.util.Map; import java.util.Random; import java.util.Set; @@ -55,6 +56,9 @@ import org.apache.hadoop.test.GenericTestUtils; import static org.apache.hadoop.util.PlatformName.IBM_JAVA; +import org.apache.log4j.AppenderSkeleton; +import org.apache.log4j.Logger; +import org.apache.log4j.spi.LoggingEvent; import org.mockito.Mockito; public class TestConfiguration extends TestCase { @@ -161,6 +165,177 @@ public class TestConfiguration extends TestCase { assertEquals("A", conf.get("prop")); } + public void testFinalWarnings() throws Exception { + // Make a configuration file with a final property + StringWriter writer = new StringWriter(); + out = new BufferedWriter(writer); + startConfig(); + declareProperty("prop", "A", "A", true); + endConfig(); + byte[] bytes = writer.toString().getBytes(); + InputStream in1 = new ByteArrayInputStream(bytes); + + // Make a second config file with a final property with a different value + writer = new StringWriter(); + out = new BufferedWriter(writer); + startConfig(); + declareProperty("prop", "BB", "BB", true); + endConfig(); + byte[] bytes2 = writer.toString().getBytes(); + InputStream in2 = new ByteArrayInputStream(bytes2); + + // Attach our own log appender so we can verify output + TestAppender appender = new TestAppender(); + final Logger logger = Logger.getRootLogger(); + logger.addAppender(appender); + + try { + // Add the 2 different resources - this should generate a warning + conf.addResource(in1); + conf.addResource(in2); + assertEquals("should see the first value", "A", conf.get("prop")); + + List events = appender.getLog(); + assertEquals("overriding a final parameter should cause logging", 1, + events.size()); + LoggingEvent loggingEvent = events.get(0); + String renderedMessage = loggingEvent.getRenderedMessage(); + assertTrue("did not see expected string inside message "+ renderedMessage, + renderedMessage.contains("an attempt to override final parameter: " + + "prop; Ignoring.")); + } finally { + // Make sure the appender is removed + logger.removeAppender(appender); + } + } + + public void testNoFinalWarnings() throws Exception { + // Make a configuration file with a final property + StringWriter writer = new StringWriter(); + out = new BufferedWriter(writer); + startConfig(); + declareProperty("prop", "A", "A", true); + endConfig(); + byte[] bytes = writer.toString().getBytes(); + // The 2 input streams both have the same config file + InputStream in1 = new ByteArrayInputStream(bytes); + InputStream in2 = new ByteArrayInputStream(bytes); + + // Attach our own log appender so we can verify output + TestAppender appender = new TestAppender(); + final Logger logger = Logger.getRootLogger(); + logger.addAppender(appender); + + try { + // Add the resource twice from a stream - should not generate warnings + conf.addResource(in1); + conf.addResource(in2); + assertEquals("A", conf.get("prop")); + + List events = appender.getLog(); + for (LoggingEvent loggingEvent : events) { + System.out.println("Event = " + loggingEvent.getRenderedMessage()); + } + assertTrue("adding same resource twice should not cause logging", + events.isEmpty()); + } finally { + // Make sure the appender is removed + logger.removeAppender(appender); + } + } + + + + public void testFinalWarningsMultiple() throws Exception { + // Make a configuration file with a repeated final property + StringWriter writer = new StringWriter(); + out = new BufferedWriter(writer); + startConfig(); + declareProperty("prop", "A", "A", true); + declareProperty("prop", "A", "A", true); + endConfig(); + byte[] bytes = writer.toString().getBytes(); + InputStream in1 = new ByteArrayInputStream(bytes); + + // Attach our own log appender so we can verify output + TestAppender appender = new TestAppender(); + final Logger logger = Logger.getRootLogger(); + logger.addAppender(appender); + + try { + // Add the resource - this should not produce a warning + conf.addResource(in1); + assertEquals("should see the value", "A", conf.get("prop")); + + List events = appender.getLog(); + for (LoggingEvent loggingEvent : events) { + System.out.println("Event = " + loggingEvent.getRenderedMessage()); + } + assertTrue("adding same resource twice should not cause logging", + events.isEmpty()); + } finally { + // Make sure the appender is removed + logger.removeAppender(appender); + } + } + + public void testFinalWarningsMultipleOverride() throws Exception { + // Make a configuration file with 2 final properties with different values + StringWriter writer = new StringWriter(); + out = new BufferedWriter(writer); + startConfig(); + declareProperty("prop", "A", "A", true); + declareProperty("prop", "BB", "BB", true); + endConfig(); + byte[] bytes = writer.toString().getBytes(); + InputStream in1 = new ByteArrayInputStream(bytes); + + // Attach our own log appender so we can verify output + TestAppender appender = new TestAppender(); + final Logger logger = Logger.getRootLogger(); + logger.addAppender(appender); + + try { + // Add the resource - this should produce a warning + conf.addResource(in1); + assertEquals("should see the value", "A", conf.get("prop")); + + List events = appender.getLog(); + assertEquals("overriding a final parameter should cause logging", 1, + events.size()); + LoggingEvent loggingEvent = events.get(0); + String renderedMessage = loggingEvent.getRenderedMessage(); + assertTrue("did not see expected string inside message "+ renderedMessage, + renderedMessage.contains("an attempt to override final parameter: " + + "prop; Ignoring.")); + } finally { + // Make sure the appender is removed + logger.removeAppender(appender); + } + } + + /** + * A simple appender for white box testing. + */ + private static class TestAppender extends AppenderSkeleton { + private final List log = new ArrayList<>(); + + @Override public boolean requiresLayout() { + return false; + } + + @Override protected void append(final LoggingEvent loggingEvent) { + log.add(loggingEvent); + } + + @Override public void close() { + } + + public List getLog() { + return new ArrayList<>(log); + } + } + /** * Tests use of multi-byte characters in property names and values. This test * round-trips multi-byte string literals through saving and loading of config