HADOOP-6521. User specified umask using deprecated dfs.umask must override server configured using new dfs.umaskmode for backward compatibility. Contributed by Suresh Srinivas.

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

CHANGES.txt

@@ -276,6 +276,10 @@ Trunk (unreleased changes)
     HADOOP-6540. Contrib unit tests have invalid XML for core-site, etc.
     (Aaron Kimball via tomwhite)
 
+    HADOOP-6521. User specified umask using deprecated dfs.umask must override
+    server configured using new dfs.umaskmode for backward compatibility.
+    (suresh)
+    
     HADOOP-6522. Fix decoding of codepoint zero in UTF8. (cutting)
 
     HADOOP-6505. Use tr rather than sed to effect literal substitution in the

src/java/org/apache/hadoop/conf/Configuration.java

@@ -295,20 +295,6 @@ public class Configuration implements Iterable<Map.Entry<String,String>>,
     return deprecatedKeyMap.containsKey(key);
   }
  
-  /**
-   * Check whether or not the deprecated key has been specified in the
-   * configuration file rather than the new key
-   * 
-   * Returns false if the specified key is not included in the deprecated
-   * key mapping.
-   * 
-   * @param oldKey Old configuration key 
-   * @return If the old configuration key was specified rather than the new one
-   */
-  public boolean deprecatedKeyWasSet(String oldKey) {
-    return isDeprecated(oldKey) && deprecatedKeyMap.get(oldKey).accessed;
-  }
-  
   /**
    * Checks for the presence of the property <code>name</code> in the
    * deprecation map. Returns the first of the list of new keys if present
@@ -1876,11 +1862,6 @@ public class Configuration implements Iterable<Map.Entry<String,String>>,
                new String[]{CommonConfigurationKeys.NET_TOPOLOGY_CONFIGURED_NODE_MAPPING_KEY});
     Configuration.addDeprecation("topology.node.switch.mapping.impl", 
                new String[]{CommonConfigurationKeys.NET_TOPOLOGY_NODE_SWITCH_MAPPING_IMPL_KEY});
-    Configuration.addDeprecation("dfs.umask", 
-               new String[]{CommonConfigurationKeys.FS_PERMISSIONS_UMASK_KEY},
-               "dfs.umask is deprecated, use " + 
-               CommonConfigurationKeys.FS_PERMISSIONS_UMASK_KEY + 
-               " with octal or symbolic specifications.");
     Configuration.addDeprecation("dfs.df.interval", 
                new String[]{CommonConfigurationKeys.FS_DF_INTERVAL_KEY});
     Configuration.addDeprecation("dfs.client.buffer.dir", 

src/java/org/apache/hadoop/fs/permission/FsPermission.java

@@ -21,6 +21,8 @@ import java.io.DataInput;
 import java.io.DataOutput;
 import java.io.IOException;
 
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
 import org.apache.hadoop.conf.Configuration;
 import org.apache.hadoop.fs.CommonConfigurationKeys;
 import org.apache.hadoop.io.Writable;
@@ -31,6 +33,8 @@ import org.apache.hadoop.io.WritableFactory;
  * A class for file/directory permissions.
  */
 public class FsPermission implements Writable {
+  private static final Log LOG = LogFactory.getLog(FsPermission.class);
+
   static final WritableFactory FACTORY = new WritableFactory() {
     public Writable newInstance() { return new FsPermission(); }
   };
@@ -199,25 +203,39 @@ public class FsPermission implements Writable {
   public static FsPermission getUMask(Configuration conf) {
     int umask = DEFAULT_UMASK;
     
-    // Attempt to pull value from configuration, trying new key first and then
-    // deprecated key, along with a warning, if not present
+    // To ensure backward compatibility first use the deprecated key.
+    // If the deprecated key is not present then check for the new key
     if(conf != null) {
       String confUmask = conf.get(UMASK_LABEL);
-      if(confUmask != null) { // UMASK_LABEL is set
-        try {
-          if(conf.deprecatedKeyWasSet(DEPRECATED_UMASK_LABEL)) 
-            umask = Integer.parseInt(confUmask); // Evaluate as decimal value
-          else
-            return new FsPermission(confUmask);
-        } catch(IllegalArgumentException iae) {
-          // Provide more explanation for user-facing message
-          String type = iae instanceof NumberFormatException ? "decimal" 
-                                                          : "octal or symbolic";
-          
-          throw new IllegalArgumentException("Unable to parse " + confUmask + 
-                                              " as " + type + " umask.");
+      int oldUmask = conf.getInt(DEPRECATED_UMASK_LABEL, Integer.MIN_VALUE);
+      try {
+        if(confUmask != null) {
+          umask = new UmaskParser(confUmask).getUMask();
+        }
+      } catch(IllegalArgumentException iae) {
+        // Provide more explanation for user-facing message
+        String type = iae instanceof NumberFormatException ? "decimal"
+            : "octal or symbolic";
+        String error = "Unable to parse configuration " + UMASK_LABEL
+            + " with value " + confUmask + " as " + type + " umask.";
+        LOG.warn(error);
+        
+        // If oldUmask is not set, then throw the exception
+        if (oldUmask == Integer.MIN_VALUE) {
+          throw new IllegalArgumentException(error);
         }
-      } 
+      }
+        
+      if(oldUmask != Integer.MIN_VALUE) { // Property was set with old key
+        if (umask != oldUmask) {
+          LOG.warn(DEPRECATED_UMASK_LABEL
+              + " configuration key is deprecated. " + "Convert to "
+              + UMASK_LABEL + ", using octal or symbolic umask "
+              + "specifications.");
+          // Old and new umask values do not match - Use old umask
+          umask = oldUmask;
+        }
+      }
     }
     
     return new FsPermission((short)umask);
@@ -229,7 +247,8 @@ public class FsPermission implements Writable {
 
   /** Set the user file creation mask (umask) */
   public static void setUMask(Configuration conf, FsPermission umask) {
-    conf.setInt(UMASK_LABEL, umask.toShort());
+    conf.set(UMASK_LABEL, String.format("%1$03o", umask.toShort()));
+    conf.setInt(DEPRECATED_UMASK_LABEL, umask.toShort());
   }
 
   /** Get the default permission. */

src/test/core/org/apache/hadoop/conf/TestConfigurationDeprecation.java

@@ -249,28 +249,4 @@ public class TestConfigurationDeprecation {
     assertNull(conf.get("I"));
     assertNull(conf.get("J"));
   }
-  
-  // Ensure that wasDeprecatedKeySet returns the correct result under
-  // the three code paths possible 
-  @Test
-  public void testWasDeprecatedKeySet() {
-    Configuration.addDeprecation("oldKeyA", new String [] { "newKeyA"});
-    Configuration.addDeprecation("oldKeyB", new String [] { "newKeyB"});
-    
-    // Used the deprecated key rather than the new, therefore should trigger
-    conf.set("oldKeyA", "AAA");
-    assertEquals("AAA", conf.get("newKeyA"));
-    assertTrue(conf.deprecatedKeyWasSet("oldKeyA"));
-  
-    // There is a deprecated key, but it wasn't specified. Therefore, don't trigger
-    conf.set("newKeyB", "AndrewBird");
-    assertEquals("AndrewBird", conf.get("newKeyB"));
-    assertFalse(conf.deprecatedKeyWasSet("oldKeyB"));
-    
-    // Not a deprecated key, therefore shouldn't trigger deprecatedKeyWasSet
-    conf.set("BrandNewKey", "BrandNewValue");
-    assertEquals("BrandNewValue", conf.get("BrandNewKey"));
-    assertFalse(conf.deprecatedKeyWasSet("BrandNewKey"));
-  }
-
 }

src/test/core/org/apache/hadoop/conf/TestDeprecatedKeys.java

@@ -20,8 +20,6 @@ package org.apache.hadoop.conf;
 
 import org.apache.hadoop.conf.Configuration;
 import org.apache.hadoop.fs.CommonConfigurationKeys;
-import org.apache.commons.logging.impl.Log4JLogger;
-import org.apache.log4j.Level;
 
 import junit.framework.TestCase;
 
@@ -33,13 +31,5 @@ public class TestDeprecatedKeys extends TestCase {
     conf.set("topology.script.file.name", "xyz");
     String scriptFile = conf.get(CommonConfigurationKeys.NET_TOPOLOGY_SCRIPT_FILE_NAME_KEY);
     assertTrue(scriptFile.equals("xyz")) ;
-    int m = conf.getInt(CommonConfigurationKeys.FS_PERMISSIONS_UMASK_KEY, CommonConfigurationKeys.FS_PERMISSIONS_UMASK_DEFAULT) ;
-    assertTrue(m == 0022) ;
-    conf.setInt("dfs.umask", 0077);
-    m = conf.getInt(CommonConfigurationKeys.FS_PERMISSIONS_UMASK_KEY, CommonConfigurationKeys.FS_PERMISSIONS_UMASK_DEFAULT) ;
-    assertTrue(m == 0077) ;
-    conf.set(CommonConfigurationKeys.FS_PERMISSIONS_UMASK_KEY, "405");
-    String umask = conf.get(CommonConfigurationKeys.FS_PERMISSIONS_UMASK_KEY);
-    assertTrue(umask.equals("405"));
   }
 }