HADOOP-9642. Configuration to resolve environment variables via ${env.VARIABLE} references (Kengo Seki via aw)

hadoop-common-project/hadoop-common/CHANGES.txt

@@ -25,7 +25,8 @@ Trunk (Unreleased)
 
   NEW FEATURES
 
-    HADOOP-6590. Add a username check for hadoop sub-commands (John Smith via aw)
+    HADOOP-6590. Add a username check for hadoop sub-commands (John Smith via
+    aw)
 
     HADOOP-11353. Add support for .hadooprc (aw)
 
@@ -41,6 +42,9 @@ Trunk (Unreleased)
 
     HADOOP-11565. Add --slaves shell option (aw)
 
+    HADOOP-9642. Configuration to resolve environment variables via
+    ${env.VARIABLE} references (Kengo Seki via aw)
+
   IMPROVEMENTS
 
     HADOOP-8017. Configure hadoop-main pom to get rid of M2E plugin execution

hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/conf/Configuration.java

@@ -146,6 +146,8 @@ import com.google.common.base.Preconditions;
  * available properties are:<ol>
  * <li>Other properties defined in this Configuration; and, if a name is
  * undefined here,</li>
+ * <li>Environment variables in {@link System#getenv()} if a name starts with
+ * "env.", or</li>
  * <li>Properties in {@link System#getProperties()}.</li>
  * </ol>
  *
@@ -160,13 +162,25 @@ import com.google.common.base.Preconditions;
  *  &lt;property&gt;
  *    &lt;name&gt;tempdir&lt;/name&gt;
  *    &lt;value&gt;${<i>basedir</i>}/tmp&lt;/value&gt;
- *  &lt;/property&gt;</pre></tt>
+ *  &lt;/property&gt;
+ *
+ *  &lt;property&gt;
+ *    &lt;name&gt;otherdir&lt;/name&gt;
+ *    &lt;value&gt;${<i>env.BASE_DIR</i>}/other&lt;/value&gt;
+ *  &lt;/property&gt;
+ *  </pre></tt>
  *
- * When <tt>conf.get("tempdir")</tt> is called, then <tt>${<i>basedir</i>}</tt>
+ * <p>When <tt>conf.get("tempdir")</tt> is called, then <tt>${<i>basedir</i>}</tt>
  * will be resolved to another property in this Configuration, while
  * <tt>${<i>user.name</i>}</tt> would then ordinarily be resolved to the value
  * of the System property with that name.
- * By default, warnings will be given to any deprecated configuration 
+ * <p>When <tt>conf.get("otherdir")</tt> is called, then <tt>${<i>env.BASE_DIR</i>}</tt>
+ * will be resolved to the value of the <tt>${<i>BASE_DIR</i>}</tt> environment variable.
+ * It supports <tt>${<i>env.NAME:-default</i>}</tt> and <tt>${<i>env.NAME-default</i>}</tt> notations.
+ * The former is resolved to “default” if <tt>${<i>NAME</i>}</tt> environment variable is undefined
+ * or its value is empty.
+ * The latter behaves the same way only if <tt>${<i>NAME</i>}</tt> is undefined.
+ * <p>By default, warnings will be given to any deprecated configuration 
  * parameters and these are suppressible by configuring
  * <tt>log4j.logger.org.apache.hadoop.conf.Configuration.deprecation</tt> in
  * log4j.properties file.
@@ -915,6 +929,7 @@ public class Configuration implements Iterable<Map.Entry<String,String>>,
    * Attempts to repeatedly expand the value {@code expr} by replacing the
    * left-most substring of the form "${var}" in the following precedence order
    * <ol>
+   *   <li>by the value of the environment variable "var" if defined</li>
    *   <li>by the value of the Java system property "var" if defined</li>
    *   <li>by the value of the configuration key "var" if defined</li>
    * </ol>
@@ -947,7 +962,31 @@ public class Configuration implements Iterable<Map.Entry<String,String>>,
           varBounds[SUB_END_IDX]);
       String val = null;
       try {
-        val = System.getProperty(var);
+        if (var.startsWith("env.") && 4 < var.length()) {
+          String v = var.substring(4);
+          int i = 0;
+          for (; i < v.length(); i++) {
+            char c = v.charAt(i);
+            if (c == ':' && i < v.length() - 1 && v.charAt(i + 1) == '-') {
+              val = getenv(v.substring(0, i));
+              if (val == null || val.length() == 0) {
+                val = v.substring(i + 2);
+              }
+              break;
+            } else if (c == '-') {
+              val = getenv(v.substring(0, i));
+              if (val == null) {
+                val = v.substring(i + 1);
+              }
+              break;
+            }
+          }
+          if (i == v.length()) {
+            val = getenv(v);
+          }
+        } else {
+          val = getProperty(var);
+        }
       } catch(SecurityException se) {
         LOG.warn("Unexpected SecurityException in Configuration", se);
       }
@@ -979,6 +1018,14 @@ public class Configuration implements Iterable<Map.Entry<String,String>>,
                                     + MAX_SUBST + " " + expr);
   }
   
+  String getenv(String name) {
+    return System.getenv(name);
+  }
+
+  String getProperty(String key) {
+    return System.getProperty(key);
+  }
+
   /**
    * Get the value of the <code>name</code> property, <code>null</code> if
    * no such property exists. If the key is deprecated, it returns the value of

hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/conf/TestConfiguration.java

@@ -50,6 +50,7 @@ import org.apache.hadoop.io.IOUtils;
 import org.apache.hadoop.net.NetUtils;
 import static org.apache.hadoop.util.PlatformName.IBM_JAVA;
 import org.codehaus.jackson.map.ObjectMapper;
+import org.mockito.Mockito;
 
 public class TestConfiguration extends TestCase {
 
@@ -148,46 +149,77 @@ public class TestConfiguration extends TestCase {
   }
 
   public void testVariableSubstitution() throws IOException {
+    // stubbing only environment dependent functions
+    Configuration mock = Mockito.spy(conf);
+    Mockito.when(mock.getProperty("user.name")).thenReturn("hadoop_user");
+    Mockito.when(mock.getenv("FILE_NAME")).thenReturn("hello");
+
     out=new BufferedWriter(new FileWriter(CONFIG));
     startConfig();
     declareProperty("my.int", "${intvar}", "42");
     declareProperty("intvar", "42", "42");
-    declareProperty("my.base", "/tmp/${user.name}", UNSPEC);
-    declareProperty("my.file", "hello", "hello");
+    declareProperty("my.base", "/tmp/${user.name}", "/tmp/hadoop_user");
+    declareProperty("my.file", "${env.FILE_NAME}", "hello");
     declareProperty("my.suffix", ".txt", ".txt");
     declareProperty("my.relfile", "${my.file}${my.suffix}", "hello.txt");
-    declareProperty("my.fullfile", "${my.base}/${my.file}${my.suffix}", UNSPEC);
+    declareProperty("my.fullfile", "${my.base}/${my.file}${my.suffix}", "/tmp/hadoop_user/hello.txt");
     // check that undefined variables are returned as-is
     declareProperty("my.failsexpand", "a${my.undefvar}b", "a${my.undefvar}b");
     endConfig();
     Path fileResource = new Path(CONFIG);
-    conf.addResource(fileResource);
+    mock.addResource(fileResource);
 
     for (Prop p : props) {
       System.out.println("p=" + p.name);
-      String gotVal = conf.get(p.name);
-      String gotRawVal = conf.getRaw(p.name);
+      String gotVal = mock.get(p.name);
+      String gotRawVal = mock.getRaw(p.name);
       assertEq(p.val, gotRawVal);
-      if (p.expectEval == UNSPEC) {
-        // expansion is system-dependent (uses System properties)
-        // can't do exact match so just check that all variables got expanded
-        assertTrue(gotVal != null && -1 == gotVal.indexOf("${"));
-      } else {
-        assertEq(p.expectEval, gotVal);
-      }
+      assertEq(p.expectEval, gotVal);
     }
       
     // check that expansion also occurs for getInt()
-    assertTrue(conf.getInt("intvar", -1) == 42);
-    assertTrue(conf.getInt("my.int", -1) == 42);
+    assertTrue(mock.getInt("intvar", -1) == 42);
+    assertTrue(mock.getInt("my.int", -1) == 42);
+  }
+
+  public void testEnvDefault() throws IOException {
+    Configuration mock = Mockito.spy(conf);
+    Mockito.when(mock.getenv("NULL_VALUE")).thenReturn(null);
+    Mockito.when(mock.getenv("EMPTY_VALUE")).thenReturn("");
+    Mockito.when(mock.getenv("SOME_VALUE")).thenReturn("some value");
+
+    out=new BufferedWriter(new FileWriter(CONFIG));
+    startConfig();
 
-    Map<String, String> results = conf.getValByRegex("^my.*file$");
-    assertTrue(results.keySet().contains("my.relfile"));
-    assertTrue(results.keySet().contains("my.fullfile"));
-    assertTrue(results.keySet().contains("my.file"));
-    assertEquals(-1, results.get("my.relfile").indexOf("${"));
-    assertEquals(-1, results.get("my.fullfile").indexOf("${"));
-    assertEquals(-1, results.get("my.file").indexOf("${"));
+    // if var is unbound, literal ${var} is returned
+    declareProperty("null1", "${env.NULL_VALUE}", "${env.NULL_VALUE}");
+    declareProperty("null2", "${env.NULL_VALUE-a}", "a");
+    declareProperty("null3", "${env.NULL_VALUE:-b}", "b");
+    declareProperty("empty1", "${env.EMPTY_VALUE}", "");
+    declareProperty("empty2", "${env.EMPTY_VALUE-c}", "");
+    declareProperty("empty3", "${env.EMPTY_VALUE:-d}", "d");
+    declareProperty("some1", "${env.SOME_VALUE}", "some value");
+    declareProperty("some2", "${env.SOME_VALUE-e}", "some value");
+    declareProperty("some3", "${env.SOME_VALUE:-f}", "some value");
+
+    // some edge cases
+    declareProperty("edge1", "${env.NULL_VALUE-g-h}", "g-h");
+    declareProperty("edge2", "${env.NULL_VALUE:-i:-j}", "i:-j");
+    declareProperty("edge3", "${env.NULL_VALUE-}", "");
+    declareProperty("edge4", "${env.NULL_VALUE:-}", "");
+    declareProperty("edge5", "${env.NULL_VALUE:}", "${env.NULL_VALUE:}");
+
+    endConfig();
+    Path fileResource = new Path(CONFIG);
+    mock.addResource(fileResource);
+
+    for (Prop p : props) {
+      System.out.println("p=" + p.name);
+      String gotVal = mock.get(p.name);
+      String gotRawVal = mock.getRaw(p.name);
+      assertEq(p.val, gotRawVal);
+      assertEq(p.expectEval, gotVal);
+    }
   }
 
   public void testFinalParam() throws IOException {
@@ -247,7 +279,6 @@ public class TestConfiguration extends TestCase {
     String expectEval;
   }
 
-  final String UNSPEC = null;
   ArrayList<Prop> props = new ArrayList<Prop>();
 
   void declareProperty(String name, String val, String expectEval)