Index: src/test/java/org/apache/hadoop/hbase/regionserver/TestCompoundConfiguration.java =================================================================== --- src/test/java/org/apache/hadoop/hbase/regionserver/TestCompoundConfiguration.java (revision 1535184) +++ src/test/java/org/apache/hadoop/hbase/regionserver/TestCompoundConfiguration.java (working copy) @@ -50,7 +50,7 @@ @Test public void testBasicFunctionality() throws ClassNotFoundException { CompoundConfiguration compoundConf = new CompoundConfiguration() - .add(baseConf); + .add(baseConf); assertEquals("1", compoundConf.get("A")); assertEquals(2, compoundConf.getInt("B", 0)); assertEquals(3, compoundConf.getInt("C", 0)); @@ -67,6 +67,29 @@ } @Test + public void testPut() { + CompoundConfiguration compoundConf = new CompoundConfiguration() + .add(baseConf); + assertEquals("1", compoundConf.get("A")); + assertEquals(2, compoundConf.getInt("B", 0)); + assertEquals(3, compoundConf.getInt("C", 0)); + assertEquals(0, compoundConf.getInt("D", 0)); + + compoundConf.set("A", "1337"); + compoundConf.set("string", "stringvalue"); + assertEquals(1337, compoundConf.getInt("A", 0)); + assertEquals("stringvalue", compoundConf.get("string")); + + // we didn't modify the base conf + assertEquals("1", baseConf.get("A")); + assertNull(baseConf.get("string")); + + // adding to the base shows up in the compound + baseConf.set("setInParent", "fromParent"); + assertEquals("fromParent", compoundConf.get("setInParent")); + } + + @Test public void testWithConfig() { Configuration conf = new Configuration(); conf.set("B", "2b"); @@ -128,9 +151,55 @@ } // verify that entries from ImmutableConfigMap's are merged in the iterator's view assertEquals(baseConfSize + 2, cnt); + + // Verify that adding map after compound configuration is modified overrides properly + CompoundConfiguration conf2 = new CompoundConfiguration(); + conf2.set("X", "modification"); + conf2.set("D", "not4"); + assertEquals("modification", conf2.get("X")); + assertEquals("not4", conf2.get("D")); + conf2.add(map); + assertEquals("4", conf2.get("D")); // map overrides } @Test + public void testWithStringMap() { + Map map = new HashMap(); + map.put("B", "2b"); + map.put("C", "33"); + map.put("D", "4"); + // unlike config, note that IBW Maps can accept null values + map.put("G", null); + + CompoundConfiguration compoundConf = new CompoundConfiguration().addStringMap(map); + assertEquals("2b", compoundConf.get("B")); + assertEquals(33, compoundConf.getInt("C", 0)); + assertEquals("4", compoundConf.get("D")); + assertEquals(4, compoundConf.getInt("D", 0)); + assertNull(compoundConf.get("E")); + assertEquals(6, compoundConf.getInt("F", 6)); + assertNull(compoundConf.get("G")); + + int cnt = 0; + for (Map.Entry entry : compoundConf) { + cnt++; + if (entry.getKey().equals("B")) assertEquals("2b", entry.getValue()); + else if (entry.getKey().equals("G")) assertEquals(null, entry.getValue()); + } + // verify that entries from ImmutableConfigMap's are merged in the iterator's view + assertEquals(4, cnt); + + // Verify that adding map after compound configuration is modified overrides properly + CompoundConfiguration conf2 = new CompoundConfiguration(); + conf2.set("X", "modification"); + conf2.set("D", "not4"); + assertEquals("modification", conf2.get("X")); + assertEquals("not4", conf2.get("D")); + conf2.addStringMap(map); + assertEquals("4", conf2.get("D")); // map overrides + } + + @Test public void testLaterConfigsOverrideEarlier() { Configuration map1 = new Configuration(false); map1.set("A", "2"); Index: src/main/java/org/apache/hadoop/hbase/regionserver/CompoundConfiguration.java =================================================================== --- src/main/java/org/apache/hadoop/hbase/regionserver/CompoundConfiguration.java (revision 1535184) +++ src/main/java/org/apache/hadoop/hbase/regionserver/CompoundConfiguration.java (working copy) @@ -19,12 +19,10 @@ */ package org.apache.hadoop.hbase.regionserver; -import java.io.DataInput; import java.io.DataOutput; -import java.io.IOException; +import java.io.IOException; import java.io.OutputStream; import java.util.ArrayList; -import java.util.Collection; import java.util.HashMap; import java.util.Iterator; import java.util.List; @@ -36,7 +34,6 @@ import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.hbase.io.ImmutableBytesWritable; import org.apache.hadoop.hbase.util.Bytes; -import org.apache.hadoop.util.StringUtils; /** * Do a shallow merge of multiple KV configuration pools. This is a very useful @@ -53,9 +50,15 @@ * This class is package private because we expect significant refactoring here * on the HBase side when certain HDFS changes are added & ubiquitous. Will * revisit expanding access at that point. + * + * WARNING: The values set in the CompoundConfiguration are do not handle Property variable + * substitution. However, if they are set in the underlying configuration substitutions are + * done. */ @InterfaceAudience.Private class CompoundConfiguration extends Configuration { + private Configuration mutableConf = null; + /** * Default Constructor. Initializes empty configuration */ @@ -78,51 +81,73 @@ * These initial APIs actually required original thought ***************************************************************************/ + static class ImmutableConfWrapper implements ImmutableConfigMap { + Configuration c; + + ImmutableConfWrapper(Configuration conf) { + c = conf; + } + + @Override + public Iterator> iterator() { + return c.iterator(); + } + + @Override + public String get(String key) { + return c.get(key); + } + + @Override + public String getRaw(String key) { + return c.getRaw(key); + } + + @Override + public Class getClassByName(String name) + throws ClassNotFoundException { + return c.getClassByName(name); + } + + @Override + public int size() { + return c.size(); + } + + @Override + public String toString() { + return c.toString(); + } + } + /** + * If set has been called, it will create a mutableConf. This converts the mutableConf to an + * immutable one and resets it to allow a new mutable conf. This is used when a new map or + * conf is added to the compound configuration to preserve proper override semantics. + */ + void freezeMutableConf() { + if (mutableConf == null) { + // do nothing if there is no current mutableConf + return; + } + + this.configs.add(0, new ImmutableConfWrapper(mutableConf)); + mutableConf = null; + } + + /** * Add Hadoop Configuration object to config list * @param conf configuration object * @return this, for builder pattern */ public CompoundConfiguration add(final Configuration conf) { + freezeMutableConf(); if (conf instanceof CompoundConfiguration) { this.configs.addAll(0, ((CompoundConfiguration) conf).configs); return this; } // put new config at the front of the list (top priority) - this.configs.add(0, new ImmutableConfigMap() { - Configuration c = conf; - - @Override - public String get(String key) { - return c.get(key); - } - - @Override - public String getRaw(String key) { - return c.getRaw(key); - } - - @Override - public Class getClassByName(String name) - throws ClassNotFoundException { - return c.getClassByName(name); - } - - @Override - public int size() { - return c.size(); - } - - @Override - public String toString() { - return c.toString(); - } - - @Override - public Iterator> iterator() { - return c.iterator(); - } - }); + this.configs.add(0, new ImmutableConfWrapper(conf)); return this; } @@ -137,6 +162,8 @@ */ public CompoundConfiguration add( final Map map) { + freezeMutableConf(); + // put new map at the front of the list (top priority) this.configs.add(0, new ImmutableConfigMap() { Map m = map; @@ -227,6 +254,54 @@ return this; } + /** + * Add String map to config list. This map is generally created by HTableDescriptor + * or HColumnDescriptor, but can be abstractly used. The added configuration + * overrides the previous ones if there are name collisions. + * + * @return this, for builder pattern + */ + public CompoundConfiguration addStringMap(final Map map) { + freezeMutableConf(); + + // put new map at the front of the list (top priority) + this.configs.add(0, new ImmutableConfigMap() { + Map m = map; + + @Override + public Iterator> iterator() { + return map.entrySet().iterator(); + } + + @Override + public String get(String key) { + return m.get(key); + } + + @Override + public String getRaw(String key) { + return get(key); + } + + @Override + public Class getClassByName(String name) + throws ClassNotFoundException { + return null; + } + + @Override + public int size() { + return m.size(); + } + + @Override + public String toString() { + return m.toString(); + } + }); + return this; + } + @Override public String toString() { StringBuffer sb = new StringBuffer(); @@ -239,6 +314,13 @@ @Override public String get(String key) { + if (mutableConf != null) { + String value = mutableConf.get(key); + if (value != null) { + return value; + } + } + for (ImmutableConfigMap m : this.configs) { String value = m.get(key); if (value != null) { @@ -250,6 +332,13 @@ @Override public String getRaw(String key) { + if (mutableConf != null) { + String value = mutableConf.getRaw(key); + if (value != null) { + return value; + } + } + for (ImmutableConfigMap m : this.configs) { String value = m.getRaw(key); if (value != null) { @@ -261,6 +350,13 @@ @Override public Class getClassByName(String name) throws ClassNotFoundException { + if (mutableConf != null) { + Class value = mutableConf.getClassByName(name); + if (value != null) { + return value; + } + } + for (ImmutableConfigMap m : this.configs) { try { Class value = m.getClassByName(name); @@ -275,9 +371,14 @@ throw new ClassNotFoundException(); } + // TODO: This method overestimates the number of configuration settings -- if a value is masked + // by an overriding config or map, it will be counted multiple times. @Override public int size() { int ret = 0; + if (mutableConf != null) { + ret += mutableConf.size(); + } for (ImmutableConfigMap m : this.configs) { ret += m.size(); } @@ -299,28 +400,36 @@ } } } + // add mutations to this CompoundConfiguration last. + if (mutableConf != null) { + Iterator> miter = mutableConf.iterator(); + while (miter.hasNext()) { + Map.Entry entry = miter.next(); + ret.put(entry.getKey(), entry.getValue()); + } + } return UnmodifiableIterator.decorate(ret.entrySet().iterator()); } - /*************************************************************************** - * You should just ignore everything below this line unless there's a bug in - * Configuration.java... + /** + * Get the value of the name. If the key is deprecated, + * it returns the value of the first key which replaces the deprecated key + * and is not null. + * If no such property exists, + * then defaultValue is returned. + + * The CompooundConfiguration does not do property substitution. To do so we need + * Configuration.getProps to be protected or package visible. Though in hadoop2 it is + * protected, in hadoop1 the method is private and not accessible. * - * Below get APIs are directly copied from Configuration.java Oh, how I wish - * this wasn't so! A tragically-sad example of why you use interfaces instead - * of inheritance. + * All of the get* methods call this overridden get method. * - * Why the duplication? We basically need to override Configuration.getProps - * or we'd need protected access to Configuration.properties so we can modify - * that pointer. There are a bunch of functions in the base Configuration that - * call getProps() and we need to use our derived version instead of the base - * version. We need to make a generic implementation that works across all - * HDFS versions. We should modify Configuration.properties in HDFS 1.0 to be - * protected, but we still need to have this code until that patch makes it to - * all the HDFS versions we support. - ***************************************************************************/ - + * @param name property name. + * @param defaultValue default value. + * @return property value, or defaultValue if the property + * doesn't exist. + **/ @Override public String get(String name, String defaultValue) { String ret = get(name); @@ -328,204 +437,24 @@ } @Override - public int getInt(String name, int defaultValue) { - String valueString = get(name); - if (valueString == null) - return defaultValue; - try { - String hexString = getHexDigits(valueString); - if (hexString != null) { - return Integer.parseInt(hexString, 16); - } - return Integer.parseInt(valueString); - } catch (NumberFormatException e) { - return defaultValue; + public void set(String name, String value) { + if (mutableConf == null) { + // not thread safe + mutableConf = new Configuration(false); // an empty configuration } + mutableConf.set(name, value); } - + + /*********************************************************************************************** + * These methods are unsupported, and no code using CompoundConfiguration depend upon them. + * Quickly abort upon any attempts to use them. + **********************************************************************************************/ @Override - public long getLong(String name, long defaultValue) { - String valueString = get(name); - if (valueString == null) - return defaultValue; - try { - String hexString = getHexDigits(valueString); - if (hexString != null) { - return Long.parseLong(hexString, 16); - } - return Long.parseLong(valueString); - } catch (NumberFormatException e) { - return defaultValue; - } - } - - protected String getHexDigits(String value) { - boolean negative = false; - String str = value; - String hexString = null; - if (value.startsWith("-")) { - negative = true; - str = value.substring(1); - } - if (str.startsWith("0x") || str.startsWith("0X")) { - hexString = str.substring(2); - if (negative) { - hexString = "-" + hexString; - } - return hexString; - } - return null; - } - - @Override - public float getFloat(String name, float defaultValue) { - String valueString = get(name); - if (valueString == null) - return defaultValue; - try { - return Float.parseFloat(valueString); - } catch (NumberFormatException e) { - return defaultValue; - } - } - - @Override - public boolean getBoolean(String name, boolean defaultValue) { - String valueString = get(name); - if ("true".equals(valueString)) - return true; - else if ("false".equals(valueString)) - return false; - else return defaultValue; - } - - @Override - public IntegerRanges getRange(String name, String defaultValue) { - return new IntegerRanges(get(name, defaultValue)); - } - - @Override - public Collection getStringCollection(String name) { - String valueString = get(name); - return StringUtils.getStringCollection(valueString); - } - - @Override - public String[] getStrings(String name) { - String valueString = get(name); - return StringUtils.getStrings(valueString); - } - - @Override - public String[] getStrings(String name, String... defaultValue) { - String valueString = get(name); - if (valueString == null) { - return defaultValue; - } else { - return StringUtils.getStrings(valueString); - } - } - - @Override - public Class[] getClasses(String name, Class... defaultValue) { - String[] classnames = getStrings(name); - if (classnames == null) - return defaultValue; - try { - Class[] classes = new Class[classnames.length]; - for (int i = 0; i < classnames.length; i++) { - classes[i] = getClassByName(classnames[i]); - } - return classes; - } catch (ClassNotFoundException e) { - throw new RuntimeException(e); - } - } - - @Override - public Class getClass(String name, Class defaultValue) { - String valueString = get(name); - if (valueString == null) - return defaultValue; - try { - return getClassByName(valueString); - } catch (ClassNotFoundException e) { - throw new RuntimeException(e); - } - } - - @Override - public Class getClass(String name, - Class defaultValue, Class xface) { - try { - Class theClass = getClass(name, defaultValue); - if (theClass != null && !xface.isAssignableFrom(theClass)) - throw new RuntimeException(theClass + " not " + xface.getName()); - else if (theClass != null) - return theClass.asSubclass(xface); - else - return null; - } catch (Exception e) { - throw new RuntimeException(e); - } - } - - /******************************************************************* - * This class is immutable. Quickly abort any attempts to alter it * - *******************************************************************/ - - @Override public void clear() { throw new UnsupportedOperationException("Immutable Configuration"); } @Override - public void set(String name, String value) { - throw new UnsupportedOperationException("Immutable Configuration"); - } - @Override - public void setIfUnset(String name, String value) { - throw new UnsupportedOperationException("Immutable Configuration"); - } - @Override - public void setInt(String name, int value) { - throw new UnsupportedOperationException("Immutable Configuration"); - } - @Override - public void setLong(String name, long value) { - throw new UnsupportedOperationException("Immutable Configuration"); - } - @Override - public void setFloat(String name, float value) { - throw new UnsupportedOperationException("Immutable Configuration"); - } - @Override - public void setBoolean(String name, boolean value) { - throw new UnsupportedOperationException("Immutable Configuration"); - } - @Override - public void setBooleanIfUnset(String name, boolean value) { - throw new UnsupportedOperationException("Immutable Configuration"); - } - @Override - public void setStrings(String name, String... values) { - throw new UnsupportedOperationException("Immutable Configuration"); - } - @Override - public void setClass(String name, Class theClass, Class xface) { - throw new UnsupportedOperationException("Immutable Configuration"); - } - @Override - public void setClassLoader(ClassLoader classLoader) { - throw new UnsupportedOperationException("Immutable Configuration"); - } - - @Override - public void readFields(DataInput in) throws IOException { - throw new UnsupportedOperationException("Immutable Configuration"); - } - - @Override public void write(DataOutput out) throws IOException { throw new UnsupportedOperationException("Immutable Configuration"); }