HADOOP-19161. Improvements

- Round tripping (with tests)
- remove synchronization with docs to say "just make immutable"
- better tests
- source enumset is copied to ensure immutability.

Change-Id: I55fb1f37a9553bcb257225f27806d341613cd2d6

Author: steveloughran

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

@@ -49,6 +49,7 @@
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
+import java.util.EnumSet;
 import java.util.Enumeration;
 import java.util.HashMap;
 import java.util.HashSet;
@@ -99,7 +100,7 @@
 import org.apache.hadoop.security.alias.CredentialProvider.CredentialEntry;
 import org.apache.hadoop.security.alias.CredentialProviderFactory;
 import org.apache.hadoop.thirdparty.com.google.common.base.Strings;
-import org.apache.hadoop.util.ConfigurationUtil;
+import org.apache.hadoop.util.ConfigurationHelper;
 import org.apache.hadoop.util.Preconditions;
 import org.apache.hadoop.util.ReflectionUtils;
 import org.apache.hadoop.util.StringInterner;
@@ -1799,12 +1800,12 @@ public <T extends Enum<T>> T getEnum(String name, T defaultValue) {
    * @throws IllegalArgumentException if one of the entries was unknown and ignoreUnknown is false,
    *           or there are two entries in the enum which differ only by case.
    */
-  public <E extends Enum<E>> Set<E> getEnumSet(
+  public <E extends Enum<E>> EnumSet<E> getEnumSet(
       final String key,
       final Class<E> enumClass,
       final boolean ignoreUnknown) throws IllegalArgumentException {
     final String value = get(key, "");
-    return ConfigurationUtil.parseEnumSet(key, value, enumClass, ignoreUnknown);
+    return ConfigurationHelper.parseEnumSet(key, value, enumClass, ignoreUnknown);
   }
 
   enum ParsedTimeDuration {

hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/impl/FlagSet.java

@@ -18,27 +18,28 @@
 
 package org.apache.hadoop.fs.impl;
 
-import java.util.Collections;
+import java.util.EnumSet;
 import java.util.Map;
+import java.util.Objects;
 import java.util.Set;
 import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.stream.Collectors;
 
 import org.apache.hadoop.conf.Configuration;
 import org.apache.hadoop.fs.StreamCapabilities;
+import org.apache.hadoop.util.ConfigurationHelper;
 import org.apache.hadoop.util.Preconditions;
 
-import static java.util.Objects.requireNonNull;
-import static org.apache.hadoop.util.ConfigurationUtil.mapEnumNamesToValues;
+import static org.apache.hadoop.util.ConfigurationHelper.mapEnumNamesToValues;
 
 /**
  * A set of flags, constructed from a configuration option or from a string,
  * with the semantics of
- * {@link org.apache.hadoop.util.ConfigurationUtil#parseEnumSet(String, String, Class, boolean)}
+ * {@link ConfigurationHelper#parseEnumSet(String, String, Class, boolean)}
  * and implementing {@link StreamCapabilities}.
- * Once declared immutable, flags cannot be changed.
- * While mutable, all getters and setters are synchronized, so
- * an instance is always thread-safe.
+ * Thread safety: there is no synchronization on a mutable {@code FlagSet}.
+ * Once declared immutable, flags cannot be changed, so they
+ * becomes implicitly thread-safe.
  */
 public final class FlagSet<E extends Enum<E>> implements StreamCapabilities {
 
@@ -61,10 +62,10 @@ public final class FlagSet<E extends Enum<E>> implements StreamCapabilities {
    * Create a FlagSet.
    * @param enumClass class of enum
    * @param prefix prefix (with trailing ".") for path capabilities probe
-   * @param flags flags
+   * @param flags flags. A copy of these are made.
    */
-  private FlagSet(final Class<E> enumClass, final String prefix, final Set<E> flags) {
-    this.flags = requireNonNull(flags);
+  private FlagSet(final Class<E> enumClass, final String prefix, final EnumSet<E> flags) {
+    this.flags = EnumSet.copyOf(flags);
     this.namesToValues = mapEnumNamesToValues(prefix, enumClass);
   }
 
@@ -73,16 +74,24 @@ private FlagSet(final Class<E> enumClass, final String prefix, final Set<E> flag
    * This is immutable.
    * @return the flags.
    */
-  public synchronized Set<E> flags() {
-    return Collections.unmodifiableSet(flags);
+  public EnumSet<E> flags() {
+    return EnumSet.copyOf(flags);
+  }
+
+  /**
+   * Probe for the FlagSet being empty.
+   * @return true if there are no flags set.
+   */
+  public boolean isEmpty() {
+    return flags.isEmpty();
   }
 
   /**
    * Is a flag enabled?
    * @param flag flag to check
    * @return true if it is in the set of enabled flags.
    */
-  public synchronized boolean enabled(final E flag) {
+  public boolean enabled(final E flag) {
     return flags.contains(flag);
   }
 
@@ -98,7 +107,7 @@ private void checkMutable() {
    * Enable a flag.
    * @param flag flag to enable.
    */
-  public synchronized void enable(final E flag) {
+  public void enable(final E flag) {
     checkMutable();
     flags.add(flag);
   }
@@ -107,7 +116,7 @@ public synchronized void enable(final E flag) {
    * Disable a flag.
    * @param flag flag to disable
    */
-  public synchronized void disable(final E flag) {
+  public void disable(final E flag) {
     checkMutable();
     flags.remove(flag);
   }
@@ -140,7 +149,7 @@ public boolean hasCapability(final String capability) {
   /**
    * Make immutable; no-op if already set.
    */
-  public synchronized void makeImmutable() {
+  public void makeImmutable() {
     immutable.set(true);
   }
 
@@ -153,18 +162,51 @@ public String toString() {
         + '}';
   }
 
+  /**
+   * Equality is based on the set.
+   * @param o
+   * @return
+   */
+  @Override
+  public boolean equals(final Object o) {
+    if (this == o) {return true;}
+    if (o == null || getClass() != o.getClass()) {return false;}
+    FlagSet<?> flagSet = (FlagSet<?>) o;
+    return Objects.equals(flags, flagSet.flags);
+  }
+
+  /**
+   * Hash code is based on the flags.
+   * @return a hash code.
+   */
+  @Override
+  public int hashCode() {
+    return Objects.hashCode(flags);
+  }
+
+  /**
+   * Convert to a string which can be then set in a configuration.
+   * This is effectively a marshalled form of the flags.
+   * @return a comma separated list of flag names.
+   */
+  public String toConfigurationString() {
+    return flags.stream()
+        .map(e -> e.name())
+        .collect(Collectors.joining(", "));
+  }
+
   /**
    * Create a FlagSet.
    * @param enumClass class of enum
    * @param prefix prefix (with trailing ".") for path capabilities probe
    * @param flags flags
-   * @return a FlagSet
    * @param <E> enum type
+   * @return a mutable FlagSet
    */
-  public static <E extends Enum<E>>  FlagSet<E> createFlagSet(
+  public static <E extends Enum<E>> FlagSet<E> createFlagSet(
       final Class<E> enumClass,
       final String prefix,
-      final Set<E> flags) {
+      final EnumSet<E> flags) {
     return new FlagSet<>(enumClass, prefix, flags);
   }
 
@@ -176,17 +218,17 @@ public static <E extends Enum<E>>  FlagSet<E> createFlagSet(
    * @param conf configuration
    * @param key key to look for
    * @param ignoreUnknown should unknown values raise an exception?
-   * @return a FlagSet
    * @param <E> enumeration type
+   * @return a mutable FlagSet
    * @throws IllegalArgumentException if one of the entries was unknown and ignoreUnknown is false,
-   *           or there are two entries in the enum which differ only by case.
+   * or there are two entries in the enum which differ only by case.
    */
-  public static <E extends Enum<E>>  FlagSet<E> buildFlagSet(
+  public static <E extends Enum<E>> FlagSet<E> buildFlagSet(
       final Class<E> enumClass,
       final Configuration conf,
       final String key,
       final boolean ignoreUnknown) {
-    final Set<E> flags = conf.getEnumSet(key, enumClass, ignoreUnknown);
+    final EnumSet<E> flags = conf.getEnumSet(key, enumClass, ignoreUnknown);
     return createFlagSet(enumClass, key + ".", flags);
   }
 

hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/ConfigurationUtil.java renamed to hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/ConfigurationHelper.java

@@ -22,7 +22,6 @@
 import java.util.HashMap;
 import java.util.Locale;
 import java.util.Map;
-import java.util.Set;
 import java.util.stream.Collectors;
 
 import org.apache.hadoop.classification.InterfaceAudience;
@@ -34,11 +33,11 @@
 import static org.apache.hadoop.util.StringUtils.getTrimmedStringCollection;
 
 /**
- * Configuration Utils to provide advanced configuration parsing.
+ * Configuration Helper class to provide advanced configuration parsing.
  * Private; external code MUST use {@link Configuration} instead
  */
 @InterfaceAudience.Private
-public final class ConfigurationUtil {
+public final class ConfigurationHelper {
 
   /**
    * Error string if there are multiple enum elements which only differ
@@ -48,7 +47,7 @@ public final class ConfigurationUtil {
   static final String ERROR_MULTIPLE_ELEMENTS_MATCHING_TO_LOWER_CASE_VALUE =
       "has multiple elements matching to lower case value";
 
-  private ConfigurationUtil() {
+  private ConfigurationHelper() {
   }
 
   /**
@@ -67,7 +66,7 @@ private ConfigurationUtil() {
    * or there are two entries in the enum which differ only by case.
    */
   @SuppressWarnings("unchecked")
-  public static <E extends Enum<E>> Set<E> parseEnumSet(final String key,
+  public static <E extends Enum<E>> EnumSet<E> parseEnumSet(final String key,
       final String valueString,
       final Class<E> enumClass,
       final boolean ignoreUnknown) throws IllegalArgumentException {