HBASE-8372 Provide mutability to CompoundConfiguration

This patch also addresses:
HBASE-8347 TestSecureLoadInc* tests hang repeatedly getting UnsupportedOperationException in hadoop2 profile


git-svn-id: https://svn.apache.org/repos/asf/hbase/trunk@1471118 13f79535-47bb-0310-9956-ffa450edef68

hbase-common/src/main/java/org/apache/hadoop/hbase/CompoundConfiguration.java

@@ -19,12 +19,10 @@
  */
 package org.apache.hadoop.hbase;
 
-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;
@@ -35,7 +33,6 @@ import org.apache.hadoop.classification.InterfaceAudience;
 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
@@ -51,9 +48,16 @@ import org.apache.hadoop.util.StringUtils;
  * configuration objects and have changes reflected everywhere. In contrast to a
  * deep merge, that requires you to explicitly know all applicable copies to
  * propagate changes.
+ * 
+ * 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
 public class CompoundConfiguration extends Configuration {
+
+  private Configuration mutableConf = null;
+
   /**
    * Default Constructor. Initializes empty configuration
    */
@@ -72,6 +76,60 @@ public class CompoundConfiguration extends Configuration {
   protected List<ImmutableConfigMap> configs
     = new ArrayList<ImmutableConfigMap>();
 
+  static class ImmutableConfWrapper implements  ImmutableConfigMap {
+    Configuration c;
+    
+    ImmutableConfWrapper(Configuration conf) {
+      c = conf;
+    }
+
+    @Override
+    public Iterator<Map.Entry<String,String>> 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.
    * The added configuration overrides the previous ones if there are name collisions.
@@ -79,45 +137,14 @@ public class CompoundConfiguration extends Configuration {
    * @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() {
+    this.configs.add(0, new ImmutableConfWrapper(conf));
-      Configuration c = conf;
-
-      @Override
-      public Iterator<Map.Entry<String,String>> 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();
-      }
-    });
     return this;
   }
 
@@ -133,6 +160,8 @@ public class CompoundConfiguration extends Configuration {
    */
   public CompoundConfiguration addWritableMap(
       final Map<ImmutableBytesWritable, ImmutableBytesWritable> map) {
+    freezeMutableConf();
+
     // put new map at the front of the list (top priority)
     this.configs.add(0, new ImmutableConfigMap() {
       Map<ImmutableBytesWritable, ImmutableBytesWritable> m = map;
@@ -192,6 +221,8 @@ public class CompoundConfiguration extends Configuration {
    * @return this, for builder pattern
    */
   public CompoundConfiguration addStringMap(final Map<String, String> map) {
+    freezeMutableConf();
+
     // put new map at the front of the list (top priority)
     this.configs.add(0, new ImmutableConfigMap() {
       Map<String, String> m = map;
@@ -242,6 +273,13 @@ public class CompoundConfiguration extends Configuration {
 
   @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) {
@@ -253,6 +291,13 @@ public class CompoundConfiguration extends Configuration {
 
   @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) {
@@ -264,6 +309,13 @@ public class CompoundConfiguration extends Configuration {
 
   @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) {
       Class<?> value = m.getClassByName(name);
       if (value != null) {
@@ -273,194 +325,51 @@ public class CompoundConfiguration extends Configuration {
     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();
     }
     return ret;
   }
 
-  /***************************************************************************
+  /**
-   * You should just ignore everything below this line unless there's a bug in
+   * Get the value of the <code>name</code>. If the key is deprecated,
-   * Configuration.java...
+   * it returns the value of the first key which replaces the deprecated key
-   *
+   * and is not null.
-   * Below get APIs are directly copied from Configuration.java Oh, how I wish
+   * If no such property exists,
-   * this wasn't so! A tragically-sad example of why you use interfaces instead
+   * then <code>defaultValue</code> is returned.
-   * of inheritance.
-   *
-   * 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.
-   ***************************************************************************/
 
+   * 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.
+   * 
+   * All of the get* methods call this overridden get method.
+   * 
+   * @param name property name.
+   * @param defaultValue default value.
+   * @return property value, or <code>defaultValue</code> if the property 
+   *         doesn't exist.                    
+   **/
   @Override
   public String get(String name, String defaultValue) {
     String ret = get(name);
     return ret == null ? defaultValue : ret;
   }
 
-  @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;
-    }
-  }
-
-  @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<String> 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 <U> Class<? extends U> getClass(String name,
-      Class<? extends U> defaultValue, Class<U> 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 Iterator<Map.Entry<String, String>> iterator() {
     Map<String, String> ret = new HashMap<String, String>();
+
+    // add in reverse order so that oldest get overridden.
     if (!configs.isEmpty()) {
       for (int i = configs.size() - 1; i >= 0; i--) {
         ImmutableConfigMap map = configs.get(i);
@@ -471,52 +380,35 @@ public class CompoundConfiguration extends Configuration {
         }
       }
     }
+
+    // add mutations to this CompoundConfiguration last.
+    if (mutableConf != null) {
+      Iterator<Map.Entry<String, String>> miter = mutableConf.iterator();
+      while (miter.hasNext()) {
+        Map.Entry<String, String> entry = miter.next();
+        ret.put(entry.getKey(), entry.getValue());
+      }
+    }
+
     return UnmodifiableIterator.decorate(ret.entrySet().iterator());
   }
 
   @Override
   public void set(String name, String value) {
-    throw new UnsupportedOperationException("Immutable Configuration");
+    if (mutableConf == null) {
-  }
+      // not thread safe
-  @Override
+      mutableConf = new Configuration(false); // an empty configuration
-  public void setIfUnset(String name, String value) {
+    }
-    throw new UnsupportedOperationException("Immutable Configuration");
+    mutableConf.set(name,  value);
-  }
-  @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");
   }
 
+  /***********************************************************************************************
+   * These methods are unsupported, and no code using CompoundConfiguration depend upon them.
+   * Quickly abort upon any attempts to use them. 
+   **********************************************************************************************/
+
   @Override
-  public void readFields(DataInput in) throws IOException {
+  public void clear() {
     throw new UnsupportedOperationException("Immutable Configuration");
   }
 

hbase-common/src/test/java/org/apache/hadoop/hbase/TestCompoundConfiguration.java

@@ -20,18 +20,15 @@
 package org.apache.hadoop.hbase;
 
 import java.util.HashMap;
-import java.util.Iterator;
 import java.util.Map;
 
+import junit.framework.TestCase;
+
 import org.apache.hadoop.conf.Configuration;
 import org.apache.hadoop.hbase.io.ImmutableBytesWritable;
 import org.apache.hadoop.hbase.util.Bytes;
-import org.apache.hadoop.hbase.SmallTests;
-
-import org.junit.experimental.categories.Category;
 import org.junit.Test;
-
+import org.junit.experimental.categories.Category;
-import junit.framework.TestCase;
 
 @Category(SmallTests.class)
 public class TestCompoundConfiguration extends TestCase {
@@ -50,7 +47,7 @@ public class TestCompoundConfiguration extends TestCase {
   @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));
@@ -66,6 +63,29 @@ public class TestCompoundConfiguration extends TestCase {
     }
   }
 
+  @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();
@@ -128,6 +148,15 @@ public class TestCompoundConfiguration extends TestCase {
     }
     // 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.addWritableMap(map);
+    assertEquals("4", conf2.get("D")); // map overrides
   }
 
   @Test
@@ -156,6 +185,15 @@ public class TestCompoundConfiguration extends TestCase {
     }
     // 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