From 0b7139d6bcfe6a4860c98b3703ee163b2f4bdb36 Mon Sep 17 00:00:00 2001 From: Robert Joseph Evans Date: Tue, 10 Jul 2012 16:49:24 +0000 Subject: [PATCH] HADOOP-8525. Provide Improved Traceability for Configuration (bobby) git-svn-id: https://svn.apache.org/repos/asf/hadoop/common/trunk@1359775 13f79535-47bb-0310-9956-ffa450edef68 --- .../hadoop-common/CHANGES.txt | 2 + .../org/apache/hadoop/conf/Configuration.java | 218 +++++++++++++----- .../hadoop/util/GenericOptionsParser.java | 16 +- .../apache/hadoop/conf/TestConfServlet.java | 2 +- .../apache/hadoop/conf/TestConfiguration.java | 42 +++- 5 files changed, 205 insertions(+), 75 deletions(-) diff --git a/hadoop-common-project/hadoop-common/CHANGES.txt b/hadoop-common-project/hadoop-common/CHANGES.txt index ece64bd296..4af20948d8 100644 --- a/hadoop-common-project/hadoop-common/CHANGES.txt +++ b/hadoop-common-project/hadoop-common/CHANGES.txt @@ -15,6 +15,8 @@ Trunk (unreleased changes) HADOOP-8470. Add NetworkTopologyWithNodeGroup, a 4-layer implementation of NetworkTopology. (Junping Du via szetszwo) + HADOOP-8525. Provide Improved Traceability for Configuration (bobby) + IMPROVEMENTS HADOOP-8017. Configure hadoop-main pom to get rid of M2E plugin execution 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 737fb8c9f0..f63e052121 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 @@ -40,6 +40,7 @@ import java.util.HashMap; import java.util.HashSet; import java.util.Iterator; +import java.util.LinkedList; import java.util.List; import java.util.ListIterator; import java.util.Map; @@ -75,7 +76,6 @@ import org.apache.hadoop.util.StringUtils; import org.codehaus.jackson.JsonFactory; import org.codehaus.jackson.JsonGenerator; -import org.w3c.dom.Comment; import org.w3c.dom.DOMException; import org.w3c.dom.Document; import org.w3c.dom.Element; @@ -158,17 +158,45 @@ public class Configuration implements Iterable>, private boolean quietmode = true; + private static class Resource { + private final Object resource; + private final String name; + + public Resource(Object resource) { + this(resource, resource.toString()); + } + + public Resource(Object resource, String name) { + this.resource = resource; + this.name = name; + } + + public String getName(){ + return name; + } + + public Object getResource() { + return resource; + } + + @Override + public String toString() { + return name; + } + } + /** * List of configuration resources. */ - private ArrayList resources = new ArrayList(); - + private ArrayList resources = new ArrayList(); + /** * The value reported as the setting resource when a key is set - * by code rather than a file resource. + * by code rather than a file resource by dumpConfiguration. */ static final String UNKNOWN_RESOURCE = "Unknown"; + /** * List of configuration parameters marked final. */ @@ -202,7 +230,7 @@ public class Configuration implements Iterable>, * Stores the mapping of key to the resource which modifies or loads * the key most recently */ - private HashMap updatingResource; + private HashMap updatingResource; /** * Class to keep the information about the keys which replace the deprecated @@ -369,7 +397,7 @@ public static boolean isDeprecated(String key) { * @return alternate name. */ private String[] getAlternateNames(String name) { - String oldName, altNames[] = null; + String altNames[] = null; DeprecatedKeyInfo keyInfo = deprecatedKeyMap.get(name); if (keyInfo == null) { altNames = (reverseDeprecatedKeyMap.get(name) != null ) ? @@ -485,7 +513,7 @@ public Configuration() { */ public Configuration(boolean loadDefaults) { this.loadDefaults = loadDefaults; - updatingResource = new HashMap(); + updatingResource = new HashMap(); synchronized(Configuration.class) { REGISTRY.put(this, null); } @@ -498,7 +526,7 @@ public Configuration(boolean loadDefaults) { */ @SuppressWarnings("unchecked") public Configuration(Configuration other) { - this.resources = (ArrayList)other.resources.clone(); + this.resources = (ArrayList) other.resources.clone(); synchronized(other) { if (other.properties != null) { this.properties = (Properties)other.properties.clone(); @@ -508,7 +536,7 @@ public Configuration(Configuration other) { this.overlay = (Properties)other.overlay.clone(); } - this.updatingResource = new HashMap(other.updatingResource); + this.updatingResource = new HashMap(other.updatingResource); } this.finalParameters = new HashSet(other.finalParameters); @@ -546,7 +574,7 @@ public static synchronized void addDefaultResource(String name) { * with that name. */ public void addResource(String name) { - addResourceObject(name); + addResourceObject(new Resource(name)); } /** @@ -560,7 +588,7 @@ public void addResource(String name) { * the classpath. */ public void addResource(URL url) { - addResourceObject(url); + addResourceObject(new Resource(url)); } /** @@ -574,7 +602,7 @@ public void addResource(URL url) { * the classpath. */ public void addResource(Path file) { - addResourceObject(file); + addResourceObject(new Resource(file)); } /** @@ -586,7 +614,21 @@ public void addResource(Path file) { * @param in InputStream to deserialize the object from. */ public void addResource(InputStream in) { - addResourceObject(in); + addResourceObject(new Resource(in)); + } + + /** + * Add a configuration resource. + * + * The properties of this resource will override properties of previously + * added resources, unless they were marked final. + * + * @param in InputStream to deserialize the object from. + * @param name the name of the resource because InputStream.toString is not + * very descriptive some times. + */ + public void addResource(InputStream in, String name) { + addResourceObject(new Resource(in, name)); } @@ -603,7 +645,7 @@ public synchronized void reloadConfiguration() { finalParameters.clear(); // clear site-limits } - private synchronized void addResourceObject(Object resource) { + private synchronized void addResourceObject(Resource resource) { resources.add(resource); // add to resources reloadConfiguration(); } @@ -721,17 +763,39 @@ public String getRaw(String name) { * @param value property value. */ public void set(String name, String value) { + set(name, value, null); + } + + /** + * Set the value of the name property. If + * name is deprecated or there is a deprecated name associated to it, + * it sets the value to both names. + * + * @param name property name. + * @param value property value. + * @param source the place that this configuration value came from + * (For debugging). + */ + public void set(String name, String value, String source) { if (deprecatedKeyMap.isEmpty()) { getProps(); } getOverlay().setProperty(name, value); getProps().setProperty(name, value); - updatingResource.put(name, UNKNOWN_RESOURCE); + if(source == null) { + updatingResource.put(name, new String[] {"programatically"}); + } else { + updatingResource.put(name, new String[] {source}); + } String[] altNames = getAlternateNames(name); if (altNames != null && altNames.length > 0) { + String altSource = "because " + name + " is deprecated"; for(String altName : altNames) { - getOverlay().setProperty(altName, value); - getProps().setProperty(altName, value); + if(!altName.equals(name)) { + getOverlay().setProperty(altName, value); + getProps().setProperty(altName, value); + updatingResource.put(altName, new String[] {altSource}); + } } } warnOnceIfDeprecated(name); @@ -1071,17 +1135,22 @@ public void setPattern(String name, Pattern pattern) { } /** - * Gets the absolute path to the resource object (file, URL, etc.), for a given - * property name. + * Gets information about why a property was set. Typically this is the + * path to the resource objects (file, URL, etc.) the property came from, but + * it can also indicate that it was set programatically, or because of the + * command line. * * @param name - The property name to get the source of. - * @return null - If the property or its source wasn't found or if the property - * was defined in code (i.e. in a Configuration instance, not from a physical - * resource). Otherwise, returns the absolute path of the resource that loaded - * the property name, as a String. + * @return null - If the property or its source wasn't found. Otherwise, + * returns a list of the sources of the resource. The older sources are + * the first ones in the list. So for example if a configuration is set from + * the command line, and then written out to a file that is read back in the + * first entry would indicate that it was set from the command line, while + * the second one would indicate the file that the new configuration was read + * in from. */ @InterfaceStability.Unstable - public synchronized String getPropertySource(String name) { + public synchronized String[] getPropertySources(String name) { if (properties == null) { // If properties is null, it means a resource was newly added // but the props were cleared so as to load it upon future @@ -1093,11 +1162,11 @@ public synchronized String getPropertySource(String name) { if (properties == null || updatingResource == null) { return null; } else { - String source = updatingResource.get(name); - if (source == null || source.equals(UNKNOWN_RESOURCE)) { + String[] source = updatingResource.get(name); + if(source == null) { return null; } else { - return source; + return Arrays.copyOf(source, source.length); } } } @@ -1702,11 +1771,14 @@ public Reader getConfResourceAsReader(String name) { protected synchronized Properties getProps() { if (properties == null) { properties = new Properties(); + HashMap backup = + new HashMap(updatingResource); loadResources(properties, resources, quietmode); if (overlay!= null) { properties.putAll(overlay); for (Map.Entry item: overlay.entrySet()) { - updatingResource.put((String) item.getKey(), UNKNOWN_RESOURCE); + String key = (String)item.getKey(); + updatingResource.put(key, backup.get(key)); } } } @@ -1752,25 +1824,25 @@ public Iterator> iterator() { } private void loadResources(Properties properties, - ArrayList resources, + ArrayList resources, boolean quiet) { if(loadDefaults) { for (String resource : defaultResources) { - loadResource(properties, resource, quiet); + loadResource(properties, new Resource(resource), quiet); } //support the hadoop-site.xml as a deprecated case if(getResource("hadoop-site.xml")!=null) { - loadResource(properties, "hadoop-site.xml", quiet); + loadResource(properties, new Resource("hadoop-site.xml"), quiet); } } - for (Object resource : resources) { + for (Resource resource : resources) { loadResource(properties, resource, quiet); } } - private void loadResource(Properties properties, Object name, boolean quiet) { + private void loadResource(Properties properties, Resource wrapper, boolean quiet) { try { DocumentBuilderFactory docBuilderFactory = DocumentBuilderFactory.newInstance(); @@ -1791,26 +1863,29 @@ private void loadResource(Properties properties, Object name, boolean quiet) { Document doc = null; Element root = null; - if (name instanceof URL) { // an URL resource - URL url = (URL)name; + Object resource = wrapper.getResource(); + String name = wrapper.getName(); + + if (resource instanceof URL) { // an URL resource + URL url = (URL)resource; if (url != null) { if (!quiet) { LOG.info("parsing " + url); } doc = builder.parse(url.toString()); } - } else if (name instanceof String) { // a CLASSPATH resource - URL url = getResource((String)name); + } else if (resource instanceof String) { // a CLASSPATH resource + URL url = getResource((String)resource); if (url != null) { if (!quiet) { LOG.info("parsing " + url); } doc = builder.parse(url.toString()); } - } else if (name instanceof Path) { // a file resource + } else if (resource instanceof Path) { // a file resource // Can't use FileSystem API or we get an infinite loop // since FileSystem uses Configuration API. Use java.io.File instead. - File file = new File(((Path)name).toUri().getPath()) + File file = new File(((Path)resource).toUri().getPath()) .getAbsoluteFile(); if (file.exists()) { if (!quiet) { @@ -1823,20 +1898,20 @@ private void loadResource(Properties properties, Object name, boolean quiet) { in.close(); } } - } else if (name instanceof InputStream) { + } else if (resource instanceof InputStream) { try { - doc = builder.parse((InputStream)name); + doc = builder.parse((InputStream)resource); } finally { - ((InputStream)name).close(); + ((InputStream)resource).close(); } - } else if (name instanceof Element) { - root = (Element)name; + } else if (resource instanceof Element) { + root = (Element)resource; } if (doc == null && root == null) { if (quiet) return; - throw new RuntimeException(name + " not found"); + throw new RuntimeException(resource + " not found"); } if (root == null) { @@ -1851,7 +1926,7 @@ private void loadResource(Properties properties, Object name, boolean quiet) { continue; Element prop = (Element)propNode; if ("configuration".equals(prop.getTagName())) { - loadResource(properties, prop, quiet); + loadResource(properties, new Resource(prop, name), quiet); continue; } if (!"property".equals(prop.getTagName())) @@ -1860,6 +1935,7 @@ private void loadResource(Properties properties, Object name, boolean quiet) { String attr = null; String value = null; boolean finalParameter = false; + LinkedList source = new LinkedList(); for (int j = 0; j < fields.getLength(); j++) { Node fieldNode = fields.item(j); if (!(fieldNode instanceof Element)) @@ -1871,7 +1947,10 @@ private void loadResource(Properties properties, Object name, boolean quiet) { value = ((Text)field.getFirstChild()).getData(); if ("final".equals(field.getTagName()) && field.hasChildNodes()) finalParameter = "true".equals(((Text)field.getFirstChild()).getData()); + if ("source".equals(field.getTagName()) && field.hasChildNodes()) + source.add(((Text)field.getFirstChild()).getData()); } + source.add(name); // Ignore this parameter if it has already been marked as 'final' if (attr != null) { @@ -1880,11 +1959,13 @@ private void loadResource(Properties properties, Object name, boolean quiet) { keyInfo.accessed = false; for (String key:keyInfo.newKeys) { // update new keys with deprecated key's value - loadProperty(properties, name, key, value, finalParameter); + loadProperty(properties, name, key, value, finalParameter, + source.toArray(new String[source.size()])); } } else { - loadProperty(properties, name, attr, value, finalParameter); + loadProperty(properties, name, attr, value, finalParameter, + source.toArray(new String[source.size()])); } } } @@ -1904,12 +1985,12 @@ private void loadResource(Properties properties, Object name, boolean quiet) { } } - private void loadProperty(Properties properties, Object name, String attr, - String value, boolean finalParameter) { + private void loadProperty(Properties properties, String name, String attr, + String value, boolean finalParameter, String[] source) { if (value != null) { if (!finalParameters.contains(attr)) { properties.setProperty(attr, value); - updatingResource.put(attr, name.toString()); + updatingResource.put(attr, source); } else if (!value.equals(properties.getProperty(attr))) { LOG.warn(name+":an attempt to override final parameter: "+attr +"; Ignoring."); @@ -1981,11 +2062,6 @@ private synchronized Document asXmlDocument() throws IOException { Element propNode = doc.createElement("property"); conf.appendChild(propNode); - if (updatingResource != null) { - Comment commentNode = doc.createComment( - "Loaded from " + updatingResource.get(name)); - propNode.appendChild(commentNode); - } Element nameNode = doc.createElement("name"); nameNode.appendChild(doc.createTextNode(name)); propNode.appendChild(nameNode); @@ -1994,6 +2070,17 @@ private synchronized Document asXmlDocument() throws IOException { valueNode.appendChild(doc.createTextNode(value)); propNode.appendChild(valueNode); + if (updatingResource != null) { + String[] sources = updatingResource.get(name); + if(sources != null) { + for(String s : sources) { + Element sourceNode = doc.createElement("source"); + sourceNode.appendChild(doc.createTextNode(s)); + propNode.appendChild(sourceNode); + } + } + } + conf.appendChild(doc.createTextNode("\n")); } return doc; @@ -2026,8 +2113,12 @@ public static void dumpConfiguration(Configuration config, config.get((String) item.getKey())); dumpGenerator.writeBooleanField("isFinal", config.finalParameters.contains(item.getKey())); - dumpGenerator.writeStringField("resource", - config.updatingResource.get(item.getKey())); + String[] resources = config.updatingResource.get(item.getKey()); + String resource = UNKNOWN_RESOURCE; + if(resources != null && resources.length > 0) { + resource = resources[0]; + } + dumpGenerator.writeStringField("resource", resource); dumpGenerator.writeEndObject(); } } @@ -2067,7 +2158,7 @@ public String toString() { toString(resources, sb); return sb.toString(); } - + private void toString(List resources, StringBuilder sb) { ListIterator i = resources.listIterator(); while (i.hasNext()) { @@ -2104,8 +2195,11 @@ public void readFields(DataInput in) throws IOException { clear(); int size = WritableUtils.readVInt(in); for(int i=0; i < size; ++i) { - set(org.apache.hadoop.io.Text.readString(in), - org.apache.hadoop.io.Text.readString(in)); + String key = org.apache.hadoop.io.Text.readString(in); + String value = org.apache.hadoop.io.Text.readString(in); + set(key, value); + String sources[] = WritableUtils.readCompressedStringArray(in); + updatingResource.put(key, sources); } } @@ -2116,6 +2210,8 @@ public void write(DataOutput out) throws IOException { for(Map.Entry item: props.entrySet()) { org.apache.hadoop.io.Text.writeString(out, (String) item.getKey()); org.apache.hadoop.io.Text.writeString(out, (String) item.getValue()); + WritableUtils.writeCompressedStringArray(out, + updatingResource.get(item.getKey())); } } diff --git a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/GenericOptionsParser.java b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/GenericOptionsParser.java index 71c2108266..808fcde2a8 100644 --- a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/GenericOptionsParser.java +++ b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/GenericOptionsParser.java @@ -268,7 +268,8 @@ private void processGeneralOptions(Configuration conf, } if (line.hasOption("jt")) { - conf.set("mapred.job.tracker", line.getOptionValue("jt")); + conf.set("mapred.job.tracker", line.getOptionValue("jt"), + "from -jt command line option"); } if (line.hasOption("conf")) { String[] values = line.getOptionValues("conf"); @@ -278,7 +279,8 @@ private void processGeneralOptions(Configuration conf, } if (line.hasOption("libjars")) { conf.set("tmpjars", - validateFiles(line.getOptionValue("libjars"), conf)); + validateFiles(line.getOptionValue("libjars"), conf), + "from -libjars command line option"); //setting libjars in client classpath URL[] libjars = getLibJars(conf); if(libjars!=null && libjars.length>0) { @@ -290,18 +292,20 @@ private void processGeneralOptions(Configuration conf, } if (line.hasOption("files")) { conf.set("tmpfiles", - validateFiles(line.getOptionValue("files"), conf)); + validateFiles(line.getOptionValue("files"), conf), + "from -files command line option"); } if (line.hasOption("archives")) { conf.set("tmparchives", - validateFiles(line.getOptionValue("archives"), conf)); + validateFiles(line.getOptionValue("archives"), conf), + "from -archives command line option"); } if (line.hasOption('D')) { String[] property = line.getOptionValues('D'); for(String prop : property) { String[] keyval = prop.split("=", 2); if (keyval.length == 2) { - conf.set(keyval[0], keyval[1]); + conf.set(keyval[0], keyval[1], "from command line"); } } } @@ -320,7 +324,7 @@ private void processGeneralOptions(Configuration conf, LOG.debug("setting conf tokensFile: " + fileName); } conf.set("mapreduce.job.credentials.json", localFs.makeQualified(p) - .toString()); + .toString(), "from -tokenCacheFile command line option"); } } diff --git a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/conf/TestConfServlet.java b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/conf/TestConfServlet.java index edc0cabece..1928de44a4 100644 --- a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/conf/TestConfServlet.java +++ b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/conf/TestConfServlet.java @@ -64,7 +64,7 @@ public void testWriteJson() throws Exception { String resource = (String)propertyInfo.get("resource"); System.err.println("k: " + key + " v: " + val + " r: " + resource); if (TEST_KEY.equals(key) && TEST_VAL.equals(val) - && Configuration.UNKNOWN_RESOURCE.equals(resource)) { + && "programatically".equals(resource)) { foundSetting = true; } } 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 63a357aab8..bae8471f8c 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 @@ -168,7 +168,8 @@ void appendProperty(String name, String val) throws IOException { appendProperty(name, val, false); } - void appendProperty(String name, String val, boolean isFinal) + void appendProperty(String name, String val, boolean isFinal, + String ... sources) throws IOException { out.write(""); out.write(""); @@ -180,6 +181,11 @@ void appendProperty(String name, String val, boolean isFinal) if (isFinal) { out.write("true"); } + for(String s : sources) { + out.write(""); + out.write(s); + out.write(""); + } out.write("\n"); } @@ -671,16 +677,38 @@ public void testPropertySource() throws IOException { Path fileResource = new Path(CONFIG); conf.addResource(fileResource); conf.set("fs.defaultFS", "value"); + String [] sources = conf.getPropertySources("test.foo"); + assertEquals(1, sources.length); assertEquals( "Resource string returned for a file-loaded property" + " must be a proper absolute path", fileResource, - new Path(conf.getPropertySource("test.foo"))); - assertEquals("Resource string returned for a set() property must be null", - null, - conf.getPropertySource("fs.defaultFS")); + new Path(sources[0])); + assertArrayEquals("Resource string returned for a set() property must be " + + "\"programatically\"", + new String[]{"programatically"}, + conf.getPropertySources("fs.defaultFS")); assertEquals("Resource string returned for an unset property must be null", - null, conf.getPropertySource("fs.defaultFoo")); + null, conf.getPropertySources("fs.defaultFoo")); + } + + public void testMultiplePropertySource() throws IOException { + out = new BufferedWriter(new FileWriter(CONFIG)); + startConfig(); + appendProperty("test.foo", "bar", false, "a", "b", "c"); + endConfig(); + Path fileResource = new Path(CONFIG); + conf.addResource(fileResource); + String [] sources = conf.getPropertySources("test.foo"); + assertEquals(4, sources.length); + assertEquals("a", sources[0]); + assertEquals("b", sources[1]); + assertEquals("c", sources[2]); + assertEquals( + "Resource string returned for a file-loaded property" + + " must be a proper absolute path", + fileResource, + new Path(sources[3])); } public void testSocketAddress() { @@ -929,7 +957,7 @@ public void testDumpConfiguration () throws IOException { confDump.put(prop.getKey(), prop); } assertEquals("value5",confDump.get("test.key6").getValue()); - assertEquals("Unknown", confDump.get("test.key4").getResource()); + assertEquals("programatically", confDump.get("test.key4").getResource()); outWriter.close(); }