HADOOP-19161. FlagSet class.

- thread safe
- can be made immutable.
- toString prints {values, like, this}
- tests

Feels ready to review now; nothing more I can
think of

Change-Id: Ia7251de59b31bf2e682f79fe9c6ddc095ec4bf92

Author: steveloughran

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

@@ -18,18 +18,27 @@
 
 package org.apache.hadoop.fs.impl;
 
+import java.util.Collections;
 import java.util.Map;
 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.Preconditions;
 
 import static java.util.Objects.requireNonNull;
 import static org.apache.hadoop.util.ConfigurationUtil.mapEnumNamesToValues;
 
 /**
- * A set of flags, constructed from a configuration option/value
+ * 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)}
  * 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.
  */
 public final class FlagSet<E extends Enum<E>> implements StreamCapabilities {
 
@@ -38,6 +47,11 @@ public final class FlagSet<E extends Enum<E>> implements StreamCapabilities {
    */
   private final Set<E> flags;
 
+  /**
+   * Is the set immutable?
+   */
+  private final AtomicBoolean immutable = new AtomicBoolean(false);
+
   /**
    * Mapping of prefixed flag names to enum values.
    */
@@ -55,36 +69,46 @@ private FlagSet(final Class<E> enumClass, final String prefix, final Set<E> flag
   }
 
   /**
-   * Get the flags.
-   * This is mutable.
+   * Get a copy of the flags.
+   * This is immutable.
    * @return the flags.
    */
-  public Set<E> flags() {
-    return flags;
+  public synchronized Set<E> flags() {
+    return Collections.unmodifiableSet(flags);
   }
 
   /**
    * Is a flag enabled?
    * @param flag flag to check
    * @return true if it is in the set of enabled flags.
    */
-  public boolean enabled(final E flag) {
+  public synchronized boolean enabled(final E flag) {
     return flags.contains(flag);
   }
 
+  /**
+   * Check for mutability before any mutating operation.
+   */
+  private void checkMutable() {
+    Preconditions.checkState(!immutable.get(),
+        "FlagSet is immutable");
+  }
+
   /**
    * Enable a flag.
    * @param flag flag to enable.
    */
-  public void enable(final E flag) {
+  public synchronized void enable(final E flag) {
+    checkMutable();
     flags.add(flag);
   }
 
   /**
    * Disable a flag.
    * @param flag flag to disable
    */
-  public void disable(final E flag) {
+  public synchronized void disable(final E flag) {
+    checkMutable();
     flags.remove(flag);
   }
 
@@ -102,7 +126,7 @@ public void set(final E flag, boolean state) {
   }
 
   /**
-   * Does is a flag enabled.
+   * Is a flag enabled?
    * @param capability string to query the stream support for.
    * @return true if the capability maps to an enum value and
    * that value is set.
@@ -113,11 +137,20 @@ public boolean hasCapability(final String capability) {
     return e != null && enabled(e);
   }
 
+  /**
+   * Make immutable; no-op if already set.
+   */
+  public synchronized void makeImmutable() {
+    immutable.set(true);
+  }
+
   @Override
   public String toString() {
-    return "FlagSet{" +
-        flags +
-        '}';
+    return "{" +
+        (flags.stream()
+            .map(e -> e.name())
+            .collect(Collectors.joining(", ")))
+        + '}';
   }
 
   /**

hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/impl/TestFlagSet.java

@@ -16,23 +16,7 @@
  * limitations under the License.
  */
 
-package org.apache.hadoop.fs.impl;/*
- * Licensed to the Apache Software Foundation (ASF) under one
- * or more contributor license agreements.  See the NOTICE file
- * distributed with this work for additional information
- * regarding copyright ownership.  The ASF licenses this file
- * to you under the Apache License, Version 2.0 (the
- * "License"); you may not use this file except in compliance
- * with the License.  You may obtain a copy of the License at
- *
- *     http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
+package org.apache.hadoop.fs.impl;
 
 import org.assertj.core.api.Assertions;
 import org.junit.Test;
@@ -52,17 +36,23 @@ public class TestFlagSet extends AbstractHadoopTestBase {
 
   public static final String KEY = "key";
 
+  /**
+   * Flagset used in tests and assertions.
+   */
   private FlagSet<SimpleEnum> flagSet =
       createFlagSet(SimpleEnum.class, KEY + ".", noneOf(SimpleEnum.class));
 
   /**
    * Simple Enums.
    * "i" is included for case tests, as it is special in turkey.
    */
-  private enum SimpleEnum { a, b }
+  private enum SimpleEnum { a, b, c }
 
+  /**
+   * Test that an entry can be enabled and disabled.
+   */
   @Test
-  public void testSingleEntry() throws Throwable {
+  public void testEntryEnableDisable() throws Throwable {
     Assertions.assertThat(flagSet.flags()).isEmpty();
     assertDisabled(SimpleEnum.a);
     flagSet.enable(SimpleEnum.a);
@@ -71,73 +61,177 @@ public void testSingleEntry() throws Throwable {
     assertDisabled(SimpleEnum.a);
   }
 
+  /**
+   * Test the setter.
+   */
   @Test
   public void testSetMethod() throws Throwable {
     Assertions.assertThat(flagSet.flags()).isEmpty();
     flagSet.set(SimpleEnum.a, true);
     assertEnabled(SimpleEnum.a);
     flagSet.set(SimpleEnum.a, false);
-    flagSet.disable(SimpleEnum.a);
+    assertDisabled(SimpleEnum.a);
+  }
+
+  /**
+   * Test mutability by making immutable and
+   * expecting setters to fail.
+   */
+  @Test
+  public void testMutability() throws Throwable {
+    flagSet.set(SimpleEnum.a, true);
+    flagSet.makeImmutable();
+    intercept(IllegalStateException.class, () ->
+        flagSet.disable(SimpleEnum.a));
+    assertEnabled(SimpleEnum.a);
+    intercept(IllegalStateException.class, () ->
+        flagSet.set(SimpleEnum.a, false));
+    assertEnabled(SimpleEnum.a);
+    // now look at the setters
+    intercept(IllegalStateException.class, () ->
+        flagSet.enable(SimpleEnum.b));
+    assertDisabled(SimpleEnum.b);
+    intercept(IllegalStateException.class, () ->
+        flagSet.set(SimpleEnum.b, true));
+    assertDisabled(SimpleEnum.b);
+  }
+
+  /**
+   * Test stringification.
+   */
+  @Test
+  public void testToString() throws Throwable {
+    assertStringValue("{}");
+    flagSet.enable(SimpleEnum.a);
+    assertStringValue("{a}");
+    flagSet.enable(SimpleEnum.b);
+    assertStringValue("{a, b}");
+  }
+
+  /**
+   * Assert that {@link FlagSet#toString()} matches the expected
+   * value.
+   * @param expected expected value
+   */
+  private void assertStringValue(final String expected) {
+    Assertions.assertThat(flagSet.toString())
+        .isEqualTo(expected);
   }
 
+  /**
+   * Test parsing from a configuration file.
+   * Multiple entries must be parsed, whitespace trimmed.
+   */
   @Test
   public void testConfEntry() throws Throwable {
+    final Configuration conf = mkConf("a\t,\nc ");
+    flagSet = buildFlagSet(SimpleEnum.class, conf, KEY, true);
+    assertEnabled(SimpleEnum.a);
+    assertEnabled(SimpleEnum.c);
+    assertDisabled(SimpleEnum.b);
+    assertHasCapability(KEY + ".a");
+    assertHasCapability(KEY + ".c");
+    assertLacksCapability(KEY + ".b");
+  }
+
+  /**
+   * Test parsing from a configuration file,
+   * where an entry is unknown: getter set to ignore
+   */
+  @Test
+  public void testConfEntryWithUnknownIgnored() throws Throwable {
     final Configuration conf = mkConf("a, unknown");
     flagSet = buildFlagSet(SimpleEnum.class, conf, KEY, true);
     assertEnabled(SimpleEnum.a);
     assertDisabled(SimpleEnum.b);
-    assertCapabilityTrue(KEY + ".a");
-    assertCapabilityFalse(KEY + ".b");
+    assertHasCapability(KEY + ".a");
+    assertLacksCapability(KEY + ".b");
   }
 
+  /**
+   * Test parsing from a configuration file where
+   * the same entry is duplicated.
+   */
   @Test
-  public void testConfUnknown() throws Throwable {
+  public void testDuplicateConfEntry() throws Throwable {
+    final Configuration conf = mkConf("a,\ta,\na");
+    flagSet = buildFlagSet(SimpleEnum.class, conf, KEY, true);
+    assertEnabled(SimpleEnum.a);
+    assertHasCapability(KEY + ".a");
+  }
+
+  /**
+   * Handle an unknown configuration value.
+   */
+  @Test
+  public void testConfUnknownFailure() throws Throwable {
     intercept(IllegalArgumentException.class, () ->
         buildFlagSet(SimpleEnum.class, mkConf("a, unknown"), KEY, false));
   }
 
+  /**
+   * Create a configuration with {@link #KEY} set to the given value.
+   * @param value value to set
+   * @return the configuration.
+   */
   private static Configuration mkConf(final String value) {
     final Configuration conf = new Configuration(false);
     conf.set(KEY, value);
     return conf;
   }
 
-  private void assertCapabilityTrue(final String capability) {
+  /**
+   * Assert that the flagset has a capability.
+   * @param capability capability to probe for
+   */
+  private void assertHasCapability(final String capability) {
     Assertions.assertThat(flagSet.hasCapability(capability))
-        .describedAs("Capabiilty of %s", capability)
+        .describedAs("Capabiilty of %s on %s", capability, flagSet)
         .isTrue();
   }
 
-  private void assertCapabilityFalse(final String capability) {
+  /**
+   * Assert that the flagset lacks a capability.
+   * @param capability capability to probe for
+   */
+  private void assertLacksCapability(final String capability) {
     Assertions.assertThat(flagSet.hasCapability(capability))
-        .describedAs("Capabiilty of %s", capability)
+        .describedAs("Capabiilty of %s on %s", capability, flagSet)
         .isFalse();
   }
 
 
+  /**
+   * Test the * binding.
+   */
   @Test
   public void testFlagSetStarEntry() throws Throwable {
     final Configuration conf = mkConf("*");
     flagSet = buildFlagSet(SimpleEnum.class, conf, KEY, false);
     assertEnabled(SimpleEnum.a);
     assertEnabled(SimpleEnum.b);
-    assertCapabilityTrue(KEY + ".a");
-    assertCapabilityTrue(KEY + ".b");
+    assertHasCapability(KEY + ".a");
+    assertHasCapability(KEY + ".b");
   }
 
   /**
    * Assert a flag is enabled.
    * @param flag flag to check
    */
   private void assertEnabled(final SimpleEnum flag) {
-    Assertions.assertThat(flagSet.enabled(flag)).describedAs("status of flag a").isTrue();
+    Assertions.assertThat(flagSet.enabled(flag))
+            .describedAs("status of flag %s in %s", flag, flagSet)
+
+        .isTrue();
   }
 
   /**
    * Assert a flag is disabled.
    * @param flag flag to check
    */
   private void assertDisabled(final SimpleEnum flag) {
-    Assertions.assertThat(flagSet.enabled(flag)).describedAs("status of flag a").isFalse();
+    Assertions.assertThat(flagSet.enabled(flag)).
+        describedAs("status of flag %s in %s", flag, flagSet)
+        .isFalse();
   }
 }

hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AFileSystem.java

@@ -745,6 +745,8 @@ public void initialize(URI name, Configuration originalConf)
       boolean performanceCreation = conf.getBoolean(FS_S3A_CREATE_PERFORMANCE,
           performanceFlags.enabled(PerformanceFlagEnum.create));
       performanceFlags.set(PerformanceFlagEnum.create, performanceCreation);
+      // freeze.
+      performanceFlags.makeImmutable();
 
       LOG.debug("{} = {}", FS_S3A_CREATE_PERFORMANCE, performanceCreation);
       allowAuthoritativePaths = S3Guard.getAuthoritativePaths(this);