HADOOP-14749. review s3guard docs & code prior to merge.

Contributed by Steve Loughran

hadoop-tools/hadoop-aws/pom.xml

@@ -170,7 +170,7 @@
                     <fs.s3a.scale.test.huge.filesize>${fs.s3a.scale.test.huge.filesize}</fs.s3a.scale.test.huge.filesize>
                     <fs.s3a.scale.test.huge.huge.partitionsize>${fs.s3a.scale.test.huge.partitionsize}</fs.s3a.scale.test.huge.huge.partitionsize>
                     <fs.s3a.scale.test.timeout>${fs.s3a.scale.test.timeout}</fs.s3a.scale.test.timeout>
-                    <!-- s3guard -->
+                    <!-- S3Guard -->
                     <fs.s3a.s3guard.test.enabled>${fs.s3a.s3guard.test.enabled}</fs.s3a.s3guard.test.enabled>
                     <fs.s3a.s3guard.test.authoritative>${fs.s3a.s3guard.test.authoritative}</fs.s3a.s3guard.test.authoritative>
                     <fs.s3a.s3guard.test.implementation>${fs.s3a.s3guard.test.implementation}</fs.s3a.s3guard.test.implementation>
@@ -216,7 +216,7 @@
                     <fs.s3a.scale.test.huge.filesize>${fs.s3a.scale.test.huge.filesize}</fs.s3a.scale.test.huge.filesize>
                     <fs.s3a.scale.test.huge.huge.partitionsize>${fs.s3a.scale.test.huge.partitionsize}</fs.s3a.scale.test.huge.huge.partitionsize>
                     <fs.s3a.scale.test.timeout>${fs.s3a.scale.test.timeout}</fs.s3a.scale.test.timeout>
-                    <!-- s3guard -->
+                    <!-- S3Guard -->
                     <fs.s3a.s3guard.test.enabled>${fs.s3a.s3guard.test.enabled}</fs.s3a.s3guard.test.enabled>
                     <fs.s3a.s3guard.test.implementation>${fs.s3a.s3guard.test.implementation}</fs.s3a.s3guard.test.implementation>
                     <fs.s3a.s3guard.test.authoritative>${fs.s3a.s3guard.test.authoritative}</fs.s3a.s3guard.test.authoritative>
@@ -262,7 +262,7 @@
                     <fs.s3a.scale.test.enabled>${fs.s3a.scale.test.enabled}</fs.s3a.scale.test.enabled>
                     <fs.s3a.scale.test.huge.filesize>${fs.s3a.scale.test.huge.filesize}</fs.s3a.scale.test.huge.filesize>
                     <fs.s3a.scale.test.timeout>${fs.s3a.scale.test.timeout}</fs.s3a.scale.test.timeout>
-                    <!-- s3guard -->
+                    <!-- S3Guard -->
                     <fs.s3a.s3guard.test.enabled>${fs.s3a.s3guard.test.enabled}</fs.s3a.s3guard.test.enabled>
                     <fs.s3a.s3guard.test.implementation>${fs.s3a.s3guard.test.implementation}</fs.s3a.s3guard.test.implementation>
                     <fs.s3a.s3guard.test.authoritative>${fs.s3a.s3guard.test.authoritative}</fs.s3a.s3guard.test.authoritative>
@@ -289,7 +289,7 @@
       </properties>
     </profile>
 
-    <!-- Turn on s3guard tests-->
+    <!-- Turn on S3Guard tests-->
     <profile>
       <id>s3guard</id>
       <activation>
@@ -302,7 +302,7 @@
       </properties>
     </profile>
 
-    <!-- Switch to dynamo DB for s3guard. Has no effect unless s3guard is enabled -->
+    <!-- Switch to DynamoDB for S3Guard. Has no effect unless S3Guard is enabled -->
     <profile>
       <id>dynamo</id>
       <activation>
@@ -315,7 +315,7 @@
       </properties>
     </profile>
 
-    <!-- Switch to DynamoDBLocal for s3guard. Has no effect unless s3guard is enabled -->
+    <!-- Switch to DynamoDBLocal for S3Guard. Has no effect unless S3Guard is enabled -->
     <profile>
       <id>dynamodblocal</id>
       <activation>
@@ -328,8 +328,8 @@
       </properties>
     </profile>
 
-    <!-- Switch s3guard from Authoritative=false to true
-     Has no effect unless s3guard is enabled -->
+    <!-- Switch S3Guard from Authoritative=false to true
+     Has no effect unless S3Guard is enabled -->
     <profile>
       <id>non-auth</id>
       <activation>

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

@@ -70,7 +70,8 @@ public class Listing {
    * @return the file status iterator
    */
   ProvidedFileStatusIterator createProvidedFileStatusIterator(
-      FileStatus[] fileStatuses, PathFilter filter,
+      FileStatus[] fileStatuses,
+      PathFilter filter,
       FileStatusAcceptor acceptor) {
     return new ProvidedFileStatusIterator(fileStatuses, filter, acceptor);
   }
@@ -356,7 +357,8 @@ public class Listing {
      * If there is data in the local filtered list, return true.
      * Else: request more data util that condition is met, or there
      * is no more remote listing data.
-     * Lastly, return true if the provided file status has left items.
+     * Lastly, return true if the {@code providedStatusIterator}
+     * has left items.
      * @return true if a call to {@link #next()} will succeed.
      * @throws IOException
      */

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

@@ -105,17 +105,6 @@ public class S3AFileStatus extends FileStatus {
     return isEmptyDirectory;
   }
 
-  /**
-   * Should not be called by clients.  Only used so {@link org.apache.hadoop
-   * .fs.s3a.s3guard.MetadataStore} can maintain this flag when caching
-   * FileStatuses on behalf of s3a.
-   * @param value for directories: TRUE / FALSE if known empty/not-empty,
-   *              UNKNOWN otherwise
-   */
-  public void setIsEmptyDirectory(Tristate value) {
-    isEmptyDirectory = value;
-  }
-
   /** Compare if this object is equal to another object.
    * @param   o the object to be compared.
    * @return  true if two file status has the same path name; false if not.

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

@@ -295,6 +295,7 @@ public class S3AFileSystem extends FileSystem {
     } catch (AmazonClientException e) {
       throw translateException("initializing ", new Path(name), e);
     }
+
   }
 
   /**
@@ -838,8 +839,8 @@ public class S3AFileSystem extends FileSystem {
       srcPaths = new HashSet<>(); // srcPaths need fast look up before put
       dstMetas = new ArrayList<>();
     }
-    // HADOOP-13761 s3guard: retries when source paths are not visible yet
-    // TODO s3guard: performance: mark destination dirs as authoritative
+    // TODO S3Guard HADOOP-13761: retries when source paths are not visible yet
+    // TODO S3Guard: performance: mark destination dirs as authoritative
 
     // Ok! Time to start
     if (srcStatus.isFile()) {
@@ -904,6 +905,8 @@ public class S3AFileSystem extends FileSystem {
         copyFile(key, newDstKey, length);
 
         if (hasMetadataStore()) {
+          // with a metadata store, the object entries need to be updated,
+          // including, potentially, the ancestors
           Path childSrc = keyToQualifiedPath(key);
           Path childDst = keyToQualifiedPath(newDstKey);
           if (objectRepresentsDirectory(key, length)) {
@@ -960,12 +963,18 @@ public class S3AFileSystem extends FileSystem {
 
   /**
    * Does this Filesystem have a metadata store?
-   * @return true if the FS has been instantiated with a metadata store
+   * @return true iff the FS has been instantiated with a metadata store
    */
   public boolean hasMetadataStore() {
     return !S3Guard.isNullMetadataStore(metadataStore);
   }
 
+  /**
+   * Get the metadata store.
+   * This will always be non-null, but may be bound to the
+   * {@code NullMetadataStore}.
+   * @return the metadata store of this FS instance
+   */
   @VisibleForTesting
   MetadataStore getMetadataStore() {
     return metadataStore;
@@ -1448,7 +1457,7 @@ public class S3AFileSystem extends FileSystem {
 
       if (status.isEmptyDirectory() == Tristate.TRUE) {
         LOG.debug("Deleting fake empty directory {}", key);
-        // HADOOP-13761 s3guard: retries here
+        // HADOOP-13761 S3Guard: retries here
         deleteObject(key);
         metadataStore.delete(f);
         instrumentation.directoryDeleted();
@@ -1466,7 +1475,7 @@ public class S3AFileSystem extends FileSystem {
             LOG.debug("Got object to delete {}", summary.getKey());
 
             if (keys.size() == MAX_ENTRIES_TO_DELETE) {
-              // HADOOP-13761 s3guard: retries
+              // TODO: HADOOP-13761 S3Guard: retries
               removeKeys(keys, true, false);
             }
           }
@@ -1475,7 +1484,7 @@ public class S3AFileSystem extends FileSystem {
             objects = continueListObjects(objects);
           } else {
             if (!keys.isEmpty()) {
-              // HADOOP-13761 s3guard: retries
+              // TODO: HADOOP-13761 S3Guard: retries
               removeKeys(keys, false, false);
             }
             break;
@@ -1742,7 +1751,7 @@ public class S3AFileSystem extends FileSystem {
    * Return a file status object that represents the path.
    * @param f The path we want information from
    * @return a FileStatus object
-   * @throws java.io.FileNotFoundException when the path does not exist;
+   * @throws FileNotFoundException when the path does not exist
    * @throws IOException on other problems.
    */
   public FileStatus getFileStatus(final Path f) throws IOException {
@@ -1750,12 +1759,12 @@ public class S3AFileSystem extends FileSystem {
   }
 
   /**
-   * Internal version of getFileStatus().
+   * Internal version of {@link #getFileStatus(Path)}.
    * @param f The path we want information from
    * @param needEmptyDirectoryFlag if true, implementation will calculate
    *        a TRUE or FALSE value for {@link S3AFileStatus#isEmptyDirectory()}
    * @return a S3AFileStatus object
-   * @throws java.io.FileNotFoundException when the path does not exist;
+   * @throws FileNotFoundException when the path does not exist
    * @throws IOException on other problems.
    */
   @VisibleForTesting
@@ -1800,19 +1809,25 @@ public class S3AFileSystem extends FileSystem {
       } catch (FileNotFoundException e) {
         return S3AFileStatus.fromFileStatus(msStatus, Tristate.TRUE);
       }
+      // entry was found, save in S3Guard
       return S3Guard.putAndReturn(metadataStore, s3FileStatus, instrumentation);
+    } else {
+      // there was no entry in S3Guard
+      // retrieve the data and update the metadata store in the process.
+      return S3Guard.putAndReturn(metadataStore,
+          s3GetFileStatus(path, key, tombstones), instrumentation);
     }
-    return S3Guard.putAndReturn(metadataStore,
-        s3GetFileStatus(path, key, tombstones), instrumentation);
   }
 
   /**
-   * Raw get file status that only uses S3.  Used to implement
-   * innerGetFileStatus, and for direct management of empty directory blobs.
+   * Raw {@code getFileStatus} that talks direct to S3.
+   * Used to implement {@link #innerGetFileStatus(Path, boolean)},
+   * and for direct management of empty directory blobs.
    * @param path Qualified path
    * @param key  Key string for the path
    * @return Status
-   * @throws IOException
+   * @throws FileNotFoundException when the path does not exist
+   * @throws IOException on other problems.
    */
   private S3AFileStatus s3GetFileStatus(final Path path, String key,
       Set<Path> tombstones) throws IOException {
@@ -1826,10 +1841,10 @@ public class S3AFileSystem extends FileSystem {
         } else {
           LOG.debug("Found exact file: normal file");
           return new S3AFileStatus(meta.getContentLength(),
-                  dateToLong(meta.getLastModified()),
-                  path,
-                  getDefaultBlockSize(path),
-                  username);
+              dateToLong(meta.getLastModified()),
+              path,
+              getDefaultBlockSize(path),
+              username);
         }
       } catch (AmazonServiceException e) {
         if (e.getStatusCode() != 404) {
@@ -1911,7 +1926,8 @@ public class S3AFileSystem extends FileSystem {
     throw new FileNotFoundException("No such file or directory: " + path);
   }
 
-  /** Helper function to determine if a collection of paths is empty
+  /**
+   * Helper function to determine if a collection of paths is empty
    * after accounting for tombstone markers (if provided).
    * @param keys Collection of path (prefixes / directories or keys).
    * @param tombstones Set of tombstone markers, or null if not applicable.
@@ -1932,7 +1948,8 @@ public class S3AFileSystem extends FileSystem {
     return true;
   }
 
-  /** Helper function to determine if a collection of object summaries is empty
+  /**
+   * Helper function to determine if a collection of object summaries is empty
    * after accounting for tombstone markers (if provided).
    * @param summaries Collection of objects as returned by listObjects.
    * @param tombstones Set of tombstone markers, or null if not applicable.
@@ -1945,14 +1962,12 @@ public class S3AFileSystem extends FileSystem {
       return summaries.isEmpty();
     }
     Collection<String> stringCollection = new ArrayList<>(summaries.size());
-    for(S3ObjectSummary summary : summaries) {
+    for (S3ObjectSummary summary : summaries) {
       stringCollection.add(summary.getKey());
     }
     return isEmptyOfKeys(stringCollection, tombstones);
   }
 
-
-
   /**
    * Raw version of {@link FileSystem#exists(Path)} which uses S3 only:
    * S3Guard MetadataStore, if any, will be skipped.
@@ -1962,7 +1977,8 @@ public class S3AFileSystem extends FileSystem {
     Path path = qualify(f);
     String key = pathToKey(path);
     try {
-      return s3GetFileStatus(path, key, null) != null;
+      s3GetFileStatus(path, key, null);
+      return true;
     } catch (FileNotFoundException e) {
       return false;
     }
@@ -2247,7 +2263,7 @@ public class S3AFileSystem extends FileSystem {
     deleteUnnecessaryFakeDirectories(p.getParent());
     Preconditions.checkArgument(length >= 0, "content length is negative");
 
-    // See note about failure semantics in s3guard.md doc.
+    // See note about failure semantics in S3Guard documentation
     try {
       if (hasMetadataStore()) {
         S3Guard.addAncestors(metadataStore, p, username);
@@ -2257,7 +2273,7 @@ public class S3AFileSystem extends FileSystem {
         S3Guard.putAndReturn(metadataStore, status, instrumentation);
       }
     } catch (IOException e) {
-      LOG.error("s3guard: Error updating MetadataStore for write to {}:",
+      LOG.error("S3Guard: Error updating MetadataStore for write to {}:",
           key, e);
       instrumentation.errorIgnored();
     }
@@ -2593,6 +2609,7 @@ public class S3AFileSystem extends FileSystem {
           cachedFilesIterator = listing.createProvidedFileStatusIterator(
               S3Guard.dirMetaToStatuses(meta), ACCEPT_ALL, acceptor);
           if (allowAuthoritative && meta != null && meta.isAuthoritative()) {
+            // metadata listing is authoritative, so return it directly
             return listing.createLocatedFileStatusIterator(cachedFilesIterator);
           }
         }
@@ -2606,8 +2623,7 @@ public class S3AFileSystem extends FileSystem {
             tombstones);
       }
     } catch (AmazonClientException e) {
-      // TODO s3guard:
-      // 1. retry on file not found exception
+      // TODO S3Guard: retry on file not found exception
       throw translateException("listFiles", path, e);
     }
   }

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

@@ -91,7 +91,7 @@ public class S3AInstrumentation {
   private final Map<String, MutableCounterLong> streamMetrics =
       new HashMap<>(30);
 
-  /** Instantiate this without caring whether or not s3guard is enabled. */
+  /** Instantiate this without caring whether or not S3Guard is enabled. */
   private final S3GuardInstrumentation s3GuardInstrumentation
       = new S3GuardInstrumentation();
 

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

@@ -142,7 +142,7 @@ public enum Statistic {
   STREAM_WRITE_QUEUE_DURATION("stream_write_queue_duration",
       "Total queue duration of all block uploads"),
 
-  // S3guard stats
+  // S3Guard stats
   S3GUARD_METADATASTORE_PUT_PATH_REQUEST(
       "s3guard_metadatastore_put_path_request",
       "s3guard metadata store put one metadata path request"),

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

@@ -24,8 +24,8 @@ import com.amazonaws.services.s3.transfer.Upload;
  * Simple struct that contains information about a S3 upload.
  */
 public class UploadInfo {
-  private Upload upload;
-  private long length;
+  private final Upload upload;
+  private final long length;
 
   public UploadInfo(Upload upload, long length) {
     this.upload = upload;

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

@@ -18,11 +18,6 @@
 
 package org.apache.hadoop.fs.s3a.s3guard;
 
-import org.apache.hadoop.classification.InterfaceAudience;
-import org.apache.hadoop.classification.InterfaceStability;
-import org.apache.hadoop.fs.FileStatus;
-import org.apache.hadoop.fs.Path;
-
 import java.net.URI;
 import java.net.URISyntaxException;
 import java.util.ArrayList;
@@ -34,6 +29,11 @@ import java.util.Set;
 import java.util.concurrent.ConcurrentHashMap;
 
 import com.google.common.base.Preconditions;
+
+import org.apache.hadoop.classification.InterfaceAudience;
+import org.apache.hadoop.classification.InterfaceStability;
+import org.apache.hadoop.fs.FileStatus;
+import org.apache.hadoop.fs.Path;
 import org.apache.hadoop.fs.s3a.Tristate;
 
 /**
@@ -61,7 +61,8 @@ public class DirListingMetadata {
    * Create a directory listing metadata container.
    *
    * @param path Path of the directory. If this path has a host component, then
-   *     all paths added later via put() must also have the same host.
+   *     all paths added later via {@link #put(FileStatus)} must also have
+   *     the same host.
    * @param listing Entries in the directory.
    * @param isAuthoritative true iff listing is the full contents of the
    *     directory, and the calling client reports that this may be cached as
@@ -257,6 +258,7 @@ public class DirListingMetadata {
     prettyPrint(sb);
     return sb.toString();
   }
+
   /**
    * Checks that child path is valid.
    * @param childPath path to check.
@@ -284,12 +286,15 @@ public class DirListingMetadata {
   }
 
   /**
-   * For Path's that are handed in directly, we assert they are in consistent
+   * For Paths that are handed in directly, we assert they are in consistent
    * format with checkPath().  For paths that are supplied embedded in
-   * FileStatus', we attempt to fill in missing scheme and host, when this
+   * FileStatus, we attempt to fill in missing scheme and host, when this
    * DirListingMetadata is associated with one.
    *
    * @return Path suitable for consistent hashtable lookups
+   * @throws NullPointerException null status argument
+   * @throws IllegalArgumentException bad status values or failure to
+   *                                  create a URI.
    */
   private Path childStatusToPathKey(FileStatus status) {
     Path p = status.getPath();

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

@@ -71,7 +71,8 @@ public interface DynamoDBClientFactory extends Configurable {
     @Override
     public AmazonDynamoDB createDynamoDBClient(String defaultRegion)
         throws IOException {
-      assert getConf() != null : "Should have been configured before usage";
+      Preconditions.checkNotNull(getConf(),
+          "Should have been configured before usage");
 
       final Configuration conf = getConf();
       final AWSCredentialsProvider credentials =

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

@@ -18,6 +18,7 @@
 
 package org.apache.hadoop.fs.s3a.s3guard;
 
+import java.io.FileNotFoundException;
 import java.io.IOException;
 import java.io.InterruptedIOException;
 import java.net.URI;
@@ -50,34 +51,32 @@ import com.amazonaws.services.dynamodbv2.model.ProvisionedThroughput;
 import com.amazonaws.services.dynamodbv2.model.ProvisionedThroughputDescription;
 import com.amazonaws.services.dynamodbv2.model.ResourceInUseException;
 import com.amazonaws.services.dynamodbv2.model.ResourceNotFoundException;
-
 import com.amazonaws.services.dynamodbv2.model.WriteRequest;
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Preconditions;
-
-import org.apache.commons.lang.StringUtils;
-import org.apache.hadoop.fs.FileStatus;
-import org.apache.hadoop.fs.s3a.Constants;
-import org.apache.hadoop.fs.s3a.S3AInstrumentation;
-import org.apache.hadoop.fs.s3a.Tristate;
-import org.apache.hadoop.io.retry.RetryPolicies;
-import org.apache.hadoop.io.retry.RetryPolicy;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
+import org.apache.commons.lang.StringUtils;
 import org.apache.hadoop.classification.InterfaceAudience;
 import org.apache.hadoop.classification.InterfaceStability;
 import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.FileStatus;
 import org.apache.hadoop.fs.FileSystem;
 import org.apache.hadoop.fs.Path;
+import org.apache.hadoop.fs.s3a.Constants;
 import org.apache.hadoop.fs.s3a.S3AFileSystem;
+import org.apache.hadoop.fs.s3a.S3AInstrumentation;
+import org.apache.hadoop.fs.s3a.Tristate;
+import org.apache.hadoop.io.retry.RetryPolicies;
+import org.apache.hadoop.io.retry.RetryPolicy;
 import org.apache.hadoop.security.UserGroupInformation;
 import org.apache.hadoop.util.ReflectionUtils;
 
-import static org.apache.hadoop.fs.s3a.s3guard.S3Guard.*;
 import static org.apache.hadoop.fs.s3a.Constants.*;
-import static org.apache.hadoop.fs.s3a.S3AUtils.*;
+import static org.apache.hadoop.fs.s3a.S3AUtils.translateException;
 import static org.apache.hadoop.fs.s3a.s3guard.PathMetadataDynamoDBTranslation.*;
+import static org.apache.hadoop.fs.s3a.s3guard.S3Guard.*;
 
 /**
  * DynamoDBMetadataStore is a {@link MetadataStore} that persists
@@ -159,7 +158,7 @@ import static org.apache.hadoop.fs.s3a.s3guard.PathMetadataDynamoDBTranslation.*
  * the S3 bucket that hosts the S3A instance.  During initialization, it checks
  * the location of the S3 bucket and creates a DynamoDB client connected to the
  * same region. The region may also be set explicitly by setting the config
- * parameter fs.s3a.s3guard.ddb.endpoint with the corresponding endpoint.
+ * parameter {@code fs.s3a.s3guard.ddb.region} to the corresponding region.
  */
 @InterfaceAudience.Private
 @InterfaceStability.Evolving
@@ -203,6 +202,7 @@ public class DynamoDBMetadataStore implements MetadataStore {
    * @param conf the file system configuration
    * @param s3Region region of the associated S3 bucket (if any).
    * @return DynamoDB instance.
+   * @throws IOException I/O error.
    */
   private static DynamoDB createDynamoDB(Configuration conf, String s3Region)
       throws IOException {
@@ -255,11 +255,18 @@ public class DynamoDBMetadataStore implements MetadataStore {
    * not explicitly relate to any S3 bucket, which be nonexistent.
    *
    * This is used to operate the metadata store directly beyond the scope of the
-   * S3AFileSystem integration, e.g. command line tools. Generally you should
-   * use {@link #initialize(FileSystem)} if given an initialized S3 file system.
+   * S3AFileSystem integration, e.g. command line tools.
+   * Generally, callers should use {@link #initialize(FileSystem)}
+   * with an initialized {@code S3AFileSystem} instance.
+   *
+   * Without a filesystem to act as a reference point, the configuration itself
+   * must declare the table name and region in the
+   * {@link Constants#S3GUARD_DDB_TABLE_NAME_KEY} and
+   * {@link Constants#S3GUARD_DDB_REGION_KEY} respectively.
    *
    * @see #initialize(FileSystem)
    * @throws IOException if there is an error
+   * @throws IllegalArgumentException if the configuration is incomplete
    */
   @Override
   public void initialize(Configuration config) throws IOException {
@@ -267,10 +274,10 @@ public class DynamoDBMetadataStore implements MetadataStore {
     // use the bucket as the DynamoDB table name if not specified in config
     tableName = conf.getTrimmed(S3GUARD_DDB_TABLE_NAME_KEY);
     Preconditions.checkArgument(!StringUtils.isEmpty(tableName),
-        "No DynamoDB table name configured!");
+        "No DynamoDB table name configured");
     region = conf.getTrimmed(S3GUARD_DDB_REGION_KEY);
     Preconditions.checkArgument(!StringUtils.isEmpty(region),
-        "No DynamoDB region configured!");
+        "No DynamoDB region configured");
     dynamoDB = createDynamoDB(conf, region);
 
     username = UserGroupInformation.getCurrentUser().getShortUserName();
@@ -279,6 +286,12 @@ public class DynamoDBMetadataStore implements MetadataStore {
     initTable();
   }
 
+  /**
+   * Set retry policy. This is driven by the value of
+   * {@link Constants#S3GUARD_DDB_MAX_RETRIES} with an exponential backoff
+   * between each attempt of {@link #MIN_RETRY_SLEEP_MSEC} milliseconds.
+   * @param config
+   */
   private void setMaxRetries(Configuration config) {
     int maxRetries = config.getInt(S3GUARD_DDB_MAX_RETRIES,
         S3GUARD_DDB_MAX_RETRIES_DEFAULT);
@@ -297,6 +310,14 @@ public class DynamoDBMetadataStore implements MetadataStore {
     innerDelete(path, false);
   }
 
+  /**
+   * Inner delete option, action based on the {@code tombstone} flag.
+   * No tombstone: delete the entry. Tombstone: create a tombstone entry.
+   * There is no check as to whether the entry exists in the table first.
+   * @param path path to delete
+   * @param tombstone flag to create a tombstone marker
+   * @throws IOException I/O error.
+   */
   private void innerDelete(Path path, boolean tombstone)
       throws IOException {
     path = checkPath(path);
@@ -414,6 +435,7 @@ public class DynamoDBMetadataStore implements MetadataStore {
     path = checkPath(path);
     LOG.debug("Listing table {} in region {}: {}", tableName, region, path);
 
+    // find the children in the table
     try {
       final QuerySpec spec = new QuerySpec()
           .withHashKey(pathToParentKeyAttribute(path))
@@ -432,10 +454,12 @@ public class DynamoDBMetadataStore implements MetadataStore {
           ? null
           : new DirListingMetadata(path, metas, false);
     } catch (AmazonClientException e) {
+      // failure, including the path not being present
       throw translateException("listChildren", path, e);
     }
   }
 
+  // build the list of all parent entries.
   Collection<PathMetadata> completeAncestry(
       Collection<PathMetadata> pathsToCreate) {
     // Key on path to allow fast lookup
@@ -677,7 +701,7 @@ public class DynamoDBMetadataStore implements MetadataStore {
       return;
     }
     LOG.info("Deleting DynamoDB table {} in region {}", tableName, region);
-    Preconditions.checkNotNull(dynamoDB, "Not connected to Dynamo");
+    Preconditions.checkNotNull(dynamoDB, "Not connected to DynamoDB");
     try {
       table.delete();
       table.waitForDelete();
@@ -766,22 +790,23 @@ public class DynamoDBMetadataStore implements MetadataStore {
         LOG.debug("Binding to table {}", tableName);
         final String status = table.describe().getTableStatus();
         switch (status) {
-          case "CREATING":
-          case "UPDATING":
-            LOG.debug("Table {} in region {} is being created/updated. This may "
-                    + "indicate that the table is being operated by another "
-                    + "concurrent thread or process. Waiting for active...",
-                tableName, region);
-            waitForTableActive(table);
-            break;
-          case "DELETING":
-            throw new IOException("DynamoDB table '" + tableName + "' is being "
-                + "deleted in region " + region);
-          case "ACTIVE":
-            break;
-          default:
-            throw new IOException("Unknown DynamoDB table status " + status
-                + ": tableName='" + tableName + "', region=" + region);
+        case "CREATING":
+        case "UPDATING":
+          LOG.debug("Table {} in region {} is being created/updated. This may"
+                  + " indicate that the table is being operated by another "
+                  + "concurrent thread or process. Waiting for active...",
+              tableName, region);
+          waitForTableActive(table);
+          break;
+        case "DELETING":
+          throw new FileNotFoundException("DynamoDB table "
+              + "'" + tableName + "' is being "
+              + "deleted in region " + region);
+        case "ACTIVE":
+          break;
+        default:
+          throw new IOException("Unknown DynamoDB table status " + status
+              + ": tableName='" + tableName + "', region=" + region);
         }
 
         final Item versionMarker = getVersionMarkerItem();
@@ -799,7 +824,8 @@ public class DynamoDBMetadataStore implements MetadataStore {
 
           createTable(capacity);
         } else {
-          throw new IOException("DynamoDB table '" + tableName + "' does not "
+          throw new FileNotFoundException("DynamoDB table "
+              + "'" + tableName + "' does not "
               + "exist in region " + region + "; auto-creation is turned off");
         }
       }

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

@@ -37,12 +37,13 @@ import java.util.Map;
 
 /**
  * This is a local, in-memory, implementation of MetadataStore.
- * This is *not* a coherent cache across processes.  It is only
+ * This is <i>not</i> a coherent cache across processes.  It is only
  * locally-coherent.
  *
- * The purpose of this is for unit testing.  It could also be used to accelerate
- * local-only operations where only one process is operating on a given object
- * store, or multiple processes are accessing a read-only storage bucket.
+ * The purpose of this is for unit and integration testing.
+ * It could also be used to accelerate local-only operations where only one
+ * process is operating on a given object store, or multiple processes are
+ * accessing a read-only storage bucket.
  *
  * This MetadataStore does not enforce filesystem rules such as disallowing
  * non-recursive removal of non-empty directories.  It is assumed the caller
@@ -53,6 +54,10 @@ public class LocalMetadataStore implements MetadataStore {
   public static final Logger LOG = LoggerFactory.getLogger(MetadataStore.class);
   // TODO HADOOP-13649: use time instead of capacity for eviction.
   public static final int DEFAULT_MAX_RECORDS = 128;
+
+  /**
+   * Maximum number of records.
+   */
   public static final String CONF_MAX_RECORDS =
       "fs.metadatastore.local.max_records";
 

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

@@ -48,7 +48,6 @@ public interface MetadataStore extends Closeable {
 
   /**
    * Performs one-time initialization of the metadata store via configuration.
-   *
    * @see #initialize(FileSystem)
    * @param conf Configuration.
    * @throws IOException if there is an error
@@ -149,8 +148,8 @@ public interface MetadataStore extends Closeable {
    *                      ().
    * @throws IOException if there is an error
    */
-  void move(Collection<Path> pathsToDelete, Collection<PathMetadata>
-      pathsToCreate) throws IOException;
+  void move(Collection<Path> pathsToDelete,
+      Collection<PathMetadata> pathsToCreate) throws IOException;
 
   /**
    * Saves metadata for exactly one path.

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

@@ -28,13 +28,14 @@ import java.util.Queue;
 import java.util.Set;
 
 import com.google.common.base.Preconditions;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
 import org.apache.hadoop.classification.InterfaceAudience;
 import org.apache.hadoop.classification.InterfaceStability;
 import org.apache.hadoop.fs.FileStatus;
 import org.apache.hadoop.fs.Path;
 import org.apache.hadoop.fs.RemoteIterator;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
 
 /**
  * {@code MetadataStoreListFilesIterator} is a {@link RemoteIterator} that

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

@@ -43,22 +43,18 @@ public class NullMetadataStore implements MetadataStore {
 
   @Override
   public void close() throws IOException {
-    return;
   }
 
   @Override
   public void delete(Path path) throws IOException {
-    return;
   }
 
   @Override
   public void forgetMetadata(Path path) throws IOException {
-    return;
   }
 
   @Override
   public void deleteSubtree(Path path) throws IOException {
-    return;
   }
 
   @Override
@@ -80,22 +76,18 @@ public class NullMetadataStore implements MetadataStore {
   @Override
   public void move(Collection<Path> pathsToDelete,
       Collection<PathMetadata> pathsToCreate) throws IOException {
-    return;
   }
 
   @Override
   public void put(PathMetadata meta) throws IOException {
-    return;
   }
 
   @Override
   public void put(Collection<PathMetadata> meta) throws IOException {
-    return;
   }
 
   @Override
   public void put(DirListingMetadata meta) throws IOException {
-    return;
   }
 
   @Override
@@ -104,7 +96,6 @@ public class NullMetadataStore implements MetadataStore {
 
   @Override
   public void prune(long modTime) {
-    return;
   }
 
   @Override

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

@@ -37,6 +37,11 @@ public class PathMetadata {
   private Tristate isEmptyDirectory;
   private boolean isDeleted;
 
+  /**
+   * Create a tombstone from the current time.
+   * @param path path to tombstone
+   * @return the entry.
+   */
   public static PathMetadata tombstone(Path path) {
     long now = System.currentTimeMillis();
     FileStatus status = new FileStatus(0, false, 0, 0, now, path);
@@ -75,6 +80,7 @@ public class PathMetadata {
   }
 
   /**
+   * Query if a directory is empty.
    * @return Tristate.TRUE if this is known to be an empty directory,
    * Tristate.FALSE if known to not be empty, and Tristate.UNKNOWN if the
    * MetadataStore does have enough information to determine either way.

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

@@ -136,7 +136,7 @@ final class PathMetadataDynamoDBTranslation {
           username, username, path);
     }
     boolean isDeleted =
-        item.hasAttribute(IS_DELETED) ? item.getBoolean(IS_DELETED) : false;
+        item.hasAttribute(IS_DELETED) && item.getBoolean(IS_DELETED);
 
     return new PathMetadata(fileStatus, Tristate.UNKNOWN, isDeleted);
   }
@@ -243,8 +243,8 @@ final class PathMetadataDynamoDBTranslation {
   }
 
   /**
-   * e.g. pathToParentKey(s3a://bucket/path/a) -> /bucket/path/a
-   * @param path
+   * e.g. {@code pathToParentKey(s3a://bucket/path/a) -> /bucket/path/a}
+   * @param path path to convert
    * @return string for parent key
    */
   static String pathToParentKey(Path path) {

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

@@ -18,8 +18,17 @@
 
 package org.apache.hadoop.fs.s3a.s3guard;
 
+import java.io.IOException;
+import java.net.URI;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.List;
+import java.util.Set;
+
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Preconditions;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 import org.apache.hadoop.classification.InterfaceAudience;
 import org.apache.hadoop.classification.InterfaceStability;
@@ -29,21 +38,13 @@ import org.apache.hadoop.fs.FileSystem;
 import org.apache.hadoop.fs.Path;
 import org.apache.hadoop.fs.s3a.S3AFileStatus;
 import org.apache.hadoop.fs.s3a.S3AInstrumentation;
-import org.apache.hadoop.fs.s3a.S3AUtils;
 import org.apache.hadoop.fs.s3a.Tristate;
 import org.apache.hadoop.util.ReflectionUtils;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
 
-import java.io.IOException;
-import java.net.URI;
-import java.util.ArrayList;
-import java.util.Collection;
-import java.util.List;
-import java.util.Set;
-
-import static org.apache.hadoop.fs.s3a.Constants.*;
-import static org.apache.hadoop.fs.s3a.Statistic.*;
+import static org.apache.hadoop.fs.s3a.Constants.S3_METADATA_STORE_IMPL;
+import static org.apache.hadoop.fs.s3a.Statistic.S3GUARD_METADATASTORE_PUT_PATH_LATENCY;
+import static org.apache.hadoop.fs.s3a.Statistic.S3GUARD_METADATASTORE_PUT_PATH_REQUEST;
+import static org.apache.hadoop.fs.s3a.S3AUtils.createUploadFileStatus;
 
 /**
  * Logic for integrating MetadataStore with S3A.
@@ -76,10 +77,6 @@ public final class S3Guard {
    * returning it.  Callers must clean up by calling
    * {@link MetadataStore#close()} when done using the MetadataStore.
    *
-   * If this fails to instantiate the class specified in config (fs.s3a
-   * .metadatastore.impl), or there is an error initializing it, this will log
-   * an error and return an instance of {@link NullMetadataStore} instead.
-   *
    * @param fs  FileSystem whose Configuration specifies which
    *            implementation to use.
    * @return Reference to new MetadataStore.
@@ -124,9 +121,9 @@ public final class S3Guard {
 
 
   /**
-   * Helper function which puts given S3AFileStatus in the MetadataStore and
-   * returns the same S3AFileStatus.
-   * @param ms MetadataStore to put() into.
+   * Helper function which puts a given S3AFileStatus into the MetadataStore and
+   * returns the same S3AFileStatus. Instrumentation monitors the put operation.
+   * @param ms MetadataStore to {@code put()} into.
    * @param status status to store
    * @param instrumentation instrumentation of the s3a file system
    * @return The same status as passed in
@@ -168,7 +165,7 @@ public final class S3Guard {
   }
 
   /**
-   * Given directory listing metadata from both the backing store, and the
+   * Given directory listing metadata from both the backing store and the
    * MetadataStore, merge the two sources of truth to create a consistent
    * view of the current directory contents, which can be returned to clients.
    *
@@ -187,7 +184,7 @@ public final class S3Guard {
       boolean isAuthoritative) throws IOException {
 
     // Fast-path for NullMetadataStore
-    if (ms instanceof NullMetadataStore) {
+    if (isNullMetadataStore(ms)) {
       return backingStatuses.toArray(new FileStatus[backingStatuses.size()]);
     }
 
@@ -216,7 +213,7 @@ public final class S3Guard {
 
       // Minor race condition here.  Multiple threads could add to this
       // mutable DirListingMetadata.  Since it is backed by a
-      // ConcurrentHashMap, the last put() wins.  I think this is ok.
+      // ConcurrentHashMap, the last put() wins.
       // More concerning is two threads racing on listStatus() and delete().
       // Any FileSystem has similar race conditions, but we could persist
       // a stale entry longer.  We could expose an atomic
@@ -245,6 +242,9 @@ public final class S3Guard {
 
   /**
    * Update MetadataStore to reflect creation of the given  directories.
+   *
+   * If an IOException is raised while trying to update the entry, this
+   * operation catches the exception and returns.
    * @param ms    MetadataStore to update.
    * @param dirs  null, or an ordered list of directories from leaf to root.
    *              E.g. if /a/ exists, and  mkdirs(/a/b/c/d) is called, this
@@ -285,7 +285,7 @@ public final class S3Guard {
         Path f = dirs.get(i);
         assertQualified(f);
         FileStatus status =
-            S3AUtils.createUploadFileStatus(f, true, 0, 0, owner);
+            createUploadFileStatus(f, true, 0, 0, owner);
 
         // We only need to put a DirListingMetadata if we are setting
         // authoritative bit
@@ -315,15 +315,16 @@ public final class S3Guard {
   }
 
   /**
-   * Helper function that records the move of directory path, adding
-   * resulting metadata to the supplied lists.  Does not store in MetadataStore.
+   * Helper function that records the move of directory paths, adding
+   * resulting metadata to the supplied lists.
+   * Does not store in MetadataStore.
    * @param ms  MetadataStore, used to make this a no-op, when it is
    *            NullMetadataStore.
    * @param srcPaths stores the source path here
    * @param dstMetas stores destination metadata here
    * @param srcPath  source path to store
    * @param dstPath  destination path to store
-   * @param owner Hadoop user name
+   * @param owner file owner to use in created records
    */
   public static void addMoveDir(MetadataStore ms, Collection<Path> srcPaths,
       Collection<PathMetadata> dstMetas, Path srcPath, Path dstPath,
@@ -331,10 +332,9 @@ public final class S3Guard {
     if (isNullMetadataStore(ms)) {
       return;
     }
-    assertQualified(srcPath);
-    assertQualified(dstPath);
-    FileStatus dstStatus = S3AUtils.createUploadFileStatus(dstPath, true, 0,
-        0, owner);
+    assertQualified(srcPath, dstPath);
+
+    FileStatus dstStatus = createUploadFileStatus(dstPath, true, 0, 0, owner);
     addMoveStatus(srcPaths, dstMetas, srcPath, dstStatus);
   }
 
@@ -349,7 +349,7 @@ public final class S3Guard {
    * @param dstPath  destination path to store
    * @param size length of file moved
    * @param blockSize  blocksize to associate with destination file
-   * @param owner Hadoop user name
+   * @param owner file owner to use in created records
    */
   public static void addMoveFile(MetadataStore ms, Collection<Path> srcPaths,
       Collection<PathMetadata> dstMetas, Path srcPath, Path dstPath,
@@ -357,9 +357,8 @@ public final class S3Guard {
     if (isNullMetadataStore(ms)) {
       return;
     }
-    assertQualified(srcPath);
-    assertQualified(dstPath);
-    FileStatus dstStatus = S3AUtils.createUploadFileStatus(dstPath, false,
+    assertQualified(srcPath, dstPath);
+    FileStatus dstStatus = createUploadFileStatus(dstPath, false,
         size, blockSize, owner);
     addMoveStatus(srcPaths, dstMetas, srcPath, dstStatus);
   }
@@ -390,9 +389,7 @@ public final class S3Guard {
       return;
     }
 
-    assertQualified(srcRoot);
-    assertQualified(srcPath);
-    assertQualified(dstPath);
+    assertQualified(srcRoot, srcPath, dstPath);
 
     if (srcPath.equals(srcRoot)) {
       LOG.debug("Skip moving ancestors of source root directory {}", srcRoot);
@@ -438,6 +435,11 @@ public final class S3Guard {
     dstMetas.add(new PathMetadata(dstStatus));
   }
 
+  /**
+   * Assert that the path is qualified with a host and scheme.
+   * @param p path to check
+   * @throws NullPointerException if either argument does not hold
+   */
   public static void assertQualified(Path p) {
     URI uri = p.toUri();
     // Paths must include bucket in case MetadataStore is shared between
@@ -447,4 +449,15 @@ public final class S3Guard {
     // I believe this should never fail, since there is a host?
     Preconditions.checkNotNull(uri.getScheme(), "Null scheme in " + uri);
   }
+
+  /**
+   * Assert that all paths are valid.
+   * @param paths path to check
+   * @throws NullPointerException if either argument does not hold
+   */
+  public static void assertQualified(Path...paths) {
+    for (Path path : paths) {
+      assertQualified(path);
+    }
+  }
 }

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

@@ -18,8 +18,23 @@
 
 package org.apache.hadoop.fs.s3a.s3guard;
 
+import java.io.FileNotFoundException;
+import java.io.IOException;
+import java.io.PrintStream;
+import java.net.URI;
+import java.net.URISyntaxException;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.TimeUnit;
+
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Preconditions;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
 import org.apache.hadoop.conf.Configuration;
 import org.apache.hadoop.conf.Configured;
 import org.apache.hadoop.fs.FileStatus;
@@ -34,25 +49,11 @@ import org.apache.hadoop.fs.shell.CommandFormat;
 import org.apache.hadoop.util.GenericOptionsParser;
 import org.apache.hadoop.util.Tool;
 import org.apache.hadoop.util.ToolRunner;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
-
-import java.io.FileNotFoundException;
-import java.io.IOException;
-import java.io.PrintStream;
-import java.net.URI;
-import java.net.URISyntaxException;
-import java.util.HashMap;
-import java.util.HashSet;
-import java.util.List;
-import java.util.Map;
-import java.util.Set;
-import java.util.concurrent.TimeUnit;
 
 import static org.apache.hadoop.fs.s3a.Constants.*;
 
 /**
- * Manage S3Guard Metadata Store.
+ * CLI to manage S3Guard Metadata Store.
  */
 public abstract class S3GuardTool extends Configured implements Tool {
   private static final Logger LOG = LoggerFactory.getLogger(S3GuardTool.class);
@@ -74,6 +75,8 @@ public abstract class S3GuardTool extends Configured implements Tool {
       "\t" + Import.NAME + " - " + Import.PURPOSE + "\n" +
       "\t" + Diff.NAME + " - " + Diff.PURPOSE + "\n" +
       "\t" + Prune.NAME + " - " + Prune.PURPOSE + "\n";
+  private static final String DATA_IN_S3_IS_PRESERVED
+      = "(all data in S3 is preserved";
 
   abstract public String getUsage();
 
@@ -100,7 +103,7 @@ public abstract class S3GuardTool extends Configured implements Tool {
    * Constructor a S3Guard tool with HDFS configuration.
    * @param conf Configuration.
    */
-  public S3GuardTool(Configuration conf) {
+  protected S3GuardTool(Configuration conf) {
     super(conf);
 
     commandFormat = new CommandFormat(0, Integer.MAX_VALUE);
@@ -253,8 +256,8 @@ public abstract class S3GuardTool extends Configured implements Tool {
   }
 
   /**
-   * Parse CLI arguments and returns the position arguments. The options are
-   * stored in {@link #commandFormat}
+   * Parse CLI arguments and returns the position arguments.
+   * The options are stored in {@link #commandFormat}
    *
    * @param args command line arguments.
    * @return the position arguments from CLI.
@@ -332,7 +335,8 @@ public abstract class S3GuardTool extends Configured implements Tool {
    */
   static class Destroy extends S3GuardTool {
     private static final String NAME = "destroy";
-    public static final String PURPOSE = "destroy metadata repository";
+    public static final String PURPOSE = "destroy Metadata Store data "
+        + DATA_IN_S3_IS_PRESERVED;
     private static final String USAGE = NAME + " [OPTIONS] [s3a://BUCKET]\n" +
         "\t" + PURPOSE + "\n\n" +
         "Common options:\n" +
@@ -367,8 +371,16 @@ public abstract class S3GuardTool extends Configured implements Tool {
         return INVALID_ARGUMENT;
       }
 
-      initMetadataStore(false);
-      Preconditions.checkState(ms != null, "Metadata store is not initialized");
+      try {
+        initMetadataStore(false);
+      } catch (FileNotFoundException e) {
+        // indication that the table was not found
+        LOG.debug("Failed to bind to store to be destroyed", e);
+        LOG.info("Metadata Store does not exist.");
+        return SUCCESS;
+      }
+
+      Preconditions.checkState(ms != null, "Metadata Store is not initialized");
 
       ms.destroy();
       LOG.info("Metadata store is deleted.");
@@ -440,8 +452,9 @@ public abstract class S3GuardTool extends Configured implements Tool {
     }
 
     /**
-     * Recursively import every path under path
+     * Recursively import every path under path.
      * @return number of items inserted into MetadataStore
+     * @throws IOException on I/O errors.
      */
     private long importDir(FileStatus status) throws IOException {
       Preconditions.checkArgument(status.isDirectory());
@@ -625,7 +638,7 @@ public abstract class S3GuardTool extends Configured implements Tool {
      * @param msDir the directory FileStatus obtained from the metadata store.
      * @param s3Dir the directory FileStatus obtained from S3.
      * @param out the output stream to generate diff results.
-     * @throws IOException
+     * @throws IOException on I/O errors.
      */
     private void compareDir(FileStatus msDir, FileStatus s3Dir,
                             PrintStream out) throws IOException {
@@ -676,7 +689,7 @@ public abstract class S3GuardTool extends Configured implements Tool {
      *
      * @param path the path to be compared.
      * @param out  the output stream to display results.
-     * @throws IOException
+     * @throws IOException on I/O errors.
      */
     private void compareRoot(Path path, PrintStream out) throws IOException {
       Path qualified = s3a.qualify(path);
@@ -732,7 +745,8 @@ public abstract class S3GuardTool extends Configured implements Tool {
   static class Prune extends S3GuardTool {
     private static final String NAME = "prune";
     public static final String PURPOSE = "truncate older metadata from " +
-        "repository";
+        "repository "
+        + DATA_IN_S3_IS_PRESERVED;;
     private static final String USAGE = NAME + " [OPTIONS] [s3a://BUCKET]\n" +
         "\t" + PURPOSE + "\n\n" +
         "Common options:\n" +
@@ -830,8 +844,8 @@ public abstract class S3GuardTool extends Configured implements Tool {
   private static void printHelp() {
     if (cmd == null) {
       System.err.println("Usage: hadoop " + USAGE);
-      System.err.println("\tperform metadata store " +
-          "administrative commands for s3a filesystem.");
+      System.err.println("\tperform S3Guard metadata store " +
+          "administrative commands.");
     } else {
       System.err.println("Usage: hadoop " + cmd.getUsage());
     }
@@ -881,7 +895,11 @@ public abstract class S3GuardTool extends Configured implements Tool {
     return ToolRunner.run(conf, cmd, otherArgs);
   }
 
-  public static void main(String[] args) throws Exception {
+  /**
+   * Main entry point. Calls {@code System.exit()} on all execution paths.
+   * @param args argument list
+   */
+  public static void main(String[] args) {
     try {
       int ret = run(args, new Configuration());
       System.exit(ret);
@@ -889,7 +907,7 @@ public abstract class S3GuardTool extends Configured implements Tool {
       System.err.println(e.getMessage());
       printHelp();
       System.exit(INVALID_ARGUMENT);
-    } catch (Exception e) {
+    } catch (Throwable e) {
       e.printStackTrace(System.err);
       System.exit(ERROR);
     }

hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/s3guard/package-info.java

@@ -1,4 +1,4 @@
-/**
+/*
  * 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

hadoop-tools/hadoop-aws/src/site/markdown/tools/hadoop-aws/index.md

@@ -1553,7 +1553,7 @@ for `fs.s3a.server-side-encryption-algorithm` is `AES256`.
 
 SSE-KMS is where the user specifies a Customer Master Key(CMK) that is used to
 encrypt the objects. The user may specify a specific CMK or leave the
-`fs.s3a.server-side-encryption-key` empty to use the default auto-generated key
+`fs.s3a.server-side-encryption.key` empty to use the default auto-generated key
 in AWS IAM.  Each CMK configured in AWS IAM is region specific, and cannot be
 used in a in a S3 bucket in a different region.  There is can also be policies
 assigned to the CMK that prohibit or restrict its use for users causing S3A

hadoop-tools/hadoop-aws/src/site/markdown/tools/hadoop-aws/s3guard.md

@@ -20,7 +20,7 @@
 
 ## Overview
 
-*S3Guard* is an experimental feature for the S3A client of the S3 Filesystem,
+*S3Guard* is an experimental feature for the S3A client of the S3 object store,
 which can use a (consistent) database as the store of metadata about objects
 in an S3 bucket.
 
@@ -34,8 +34,8 @@ processes.
 1. Permits a consistent view of the object store. Without this, changes in
 objects may not be immediately visible, especially in listing operations.
 
-1. Create a platform for future performance improvements for running Hadoop
-   workloads on top of object stores
+1. Offers a platform for future performance improvements for running Hadoop
+workloads on top of object stores
 
 The basic idea is that, for each operation in the Hadoop S3 client (s3a) that
 reads or modifies metadata, a shadow copy of that metadata is stored in a
@@ -60,19 +60,22 @@ S3 Repository to use the feature. Clients reading the data may work directly
 with the S3A data, in which case the normal S3 consistency guarantees apply.
 
 
-## Configuring S3Guard
+## Setting up S3Guard
 
 The latest configuration parameters are defined in `core-default.xml`.  You
 should consult that file for full information, but a summary is provided here.
 
 
-### 1. Choose your MetadataStore implementation.
+### 1. Choose the Database
 
-By default, S3Guard is not enabled.  S3A uses "`NullMetadataStore`", which is a
-MetadataStore that does nothing.
+A core concept of S3Guard is that the directory listing data of the object
+store, *the metadata* is replicated in a higher-performance, consistent,
+database. In S3Guard, this database is called *The Metadata Store*
 
-The funtional MetadataStore back-end uses Amazon's DynamoDB database service.  The
- following setting will enable this MetadataStore:
+By default, S3Guard is not enabled.
+
+The Metadata Store to use in production is bonded to Amazon's DynamoDB
+database service.  The following setting will enable this Metadata Store:
 
 ```xml
 <property>
@@ -81,8 +84,8 @@ The funtional MetadataStore back-end uses Amazon's DynamoDB database service.  T
 </property>
 ```
 
-
-Note that the Null metadata store can be explicitly requested if desired.
+Note that the `NullMetadataStore` store can be explicitly requested if desired.
+This offers no metadata storage, and effectively disables S3Guard.
 
 ```xml
 <property>
@@ -91,10 +94,10 @@ Note that the Null metadata store can be explicitly requested if desired.
 </property>
 ```
 
-### 2. Configure S3Guard settings
+### 2. Configure S3Guard Settings
 
-More settings will be added here in the future as we add to S3Guard.
-Currently the only MetadataStore-independent setting, besides the
+More settings will may be added in the future.
+Currently the only Metadata Store-independent setting, besides the
 implementation class above, is the *allow authoritative* flag.
 
 It is recommended that you leave the default setting here:
@@ -107,25 +110,32 @@ It is recommended that you leave the default setting here:
 
 ```
 
-Setting this to true is currently an experimental feature.  When true, the
+Setting this to `true` is currently an experimental feature.  When true, the
 S3A client will avoid round-trips to S3 when getting directory listings, if
-there is a fully-cached version of the directory stored in the MetadataStore.
+there is a fully-cached version of the directory stored in the Metadata Store.
 
 Note that if this is set to true, it may exacerbate or persist existing race
 conditions around multiple concurrent modifications and listings of a given
 directory tree.
 
+In particular: **If the Metadata Store is declared as authoritative,
+all interactions with the S3 bucket(s) must be through S3A clients sharing
+the same Metadata Store**
+
 
-### 3. Configure the MetadataStore.
+### 3. Configure the Metadata Store.
 
-Here are the `DynamoDBMetadataStore` settings.  Other MetadataStore
- implementations will have their own configuration parameters.
+Here are the `DynamoDBMetadataStore` settings.  Other Metadata Store
+implementations will have their own configuration parameters.
+
+
+### 4. Name Your Table
 
 First, choose the name of the table you wish to use for the S3Guard metadata
-storage in your DynamoDB instance.  If you leave the default blank value, a
+storage in your DynamoDB instance.  If you leave it unset/empty, a
 separate table will be created for each S3 bucket you access, and that
-bucket's name will be used for the name of the DynamoDB table.  Here we
-choose our own table name:
+bucket's name will be used for the name of the DynamoDB table.  For example,
+this sets the table name to `my-ddb-table-name`
 
 ```xml
 <property>
@@ -133,16 +143,45 @@ choose our own table name:
   <value>my-ddb-table-name</value>
   <description>
     The DynamoDB table name to operate. Without this property, the respective
-    S3 bucket name will be used.
+    S3 bucket names will be used.
   </description>
 </property>
 ```
 
+It is good to share a table across multiple buckets for multiple reasons.
+
+1. You are billed for the I/O capacity allocated to the table,
+*even when the table is not used*. Sharing capacity can reduce costs.
+
+1. You can share the "provision burden" across the buckets. That is, rather
+than allocating for the peak load on a single bucket, you can allocate for
+the peak load *across all the buckets*, which is likely to be significantly
+lower.
+
+1. It's easier to measure and tune the load requirements and cost of
+S3Guard, because there is only one table to review and configure in the
+AWS management console.
+
+When wouldn't you want to share a table?
+
+1. When you do explicitly want to provision I/O capacity to a specific bucket
+and table, isolated from others.
+
+1. When you are using separate billing for specific buckets allocated
+to specific projects.
+
+1. When different users/roles have different access rights to different buckets.
+As S3Guard requires all users to have R/W access to the table, all users will
+be able to list the metadata in all buckets, even those to which they lack
+read access.
+
+### 5. Locate your Table
+
 You may also wish to specify the region to use for DynamoDB.  If a region
 is not configured, S3A will assume that it is in the same region as the S3
 bucket. A list of regions for the DynamoDB service can be found in
 [Amazon's documentation](http://docs.aws.amazon.com/general/latest/gr/rande.html#ddb_region).
-In this example, we set the US West 2 region:
+In this example, to use the US West 2 region:
 
 ```xml
 <property>
@@ -151,9 +190,17 @@ In this example, we set the US West 2 region:
 </property>
 ```
 
+When working with S3Guard-managed buckets from EC2 VMs running in AWS
+infrastructure, using a local DynamoDB region ensures the lowest latency
+and highest reliability, as well as avoiding all long-haul network charges.
+The S3Guard tables, and indeed, the S3 buckets, should all be in the same
+region as the VMs.
+
+### 6. Optional: Create your Table
+
 Next, you can choose whether or not the table will be automatically created
-(if it doesn't already exist).  If we want this feature, we can set the
-following parameter to true.
+(if it doesn't already exist).  If you want this feature, set the
+`fs.s3a.s3guard.ddb.table.create` option to `true`.
 
 ```xml
 <property>
@@ -165,6 +212,8 @@ following parameter to true.
 </property>
 ```
 
+### 7. If creating a table: Set your DynamoDB IO Capacity
+
 Next, you need to set the DynamoDB read and write throughput requirements you
 expect to need for your cluster.  Setting higher values will cost you more
 money.  *Note* that these settings only affect table creation when
@@ -174,10 +223,10 @@ an existing table, use the AWS console or CLI tool.
 For more details on DynamoDB capacity units, see the AWS page on [Capacity
 Unit Calculations](http://docs.aws.amazon.com/amazondynamodb/latest/developerguide/WorkingWithTables.html#CapacityUnitCalculations).
 
-The charges are incurred per hour for the life of the table, even when the
-table and the underlying S3 bucket are not being used.
+The charges are incurred per hour for the life of the table, *even when the
+table and the underlying S3 buckets are not being used*.
 
-There are also charges incurred for data storage and  for data IO outside of the
+There are also charges incurred for data storage and for data IO outside of the
 region of the DynamoDB instance. S3Guard only stores metadata in DynamoDB: path names
 and summary details of objects —the actual data is stored in S3, so billed at S3
 rates.
@@ -210,29 +259,107 @@ Attempting to perform more IO than the capacity requested simply throttles the
 IO; small capacity numbers are recommended when initially experimenting
 with S3Guard.
 
-## Credentials
+## Authenticating with S3Guard
 
 The DynamoDB metadata store takes advantage of the fact that the DynamoDB
-service uses uses the same authentication mechanisms as S3. With S3Guard,
-DynamoDB doesn't have any dedicated authentication configuration; it gets its
-credentials from the S3A client that is using it.
+service uses the same authentication mechanisms as S3. S3Guard
+gets all its credentials from the S3A client that is using it.
+
+All existing S3 authentication mechanisms can be used, except for one
+exception. Credentials placed in URIs are not supported for S3Guard, for security
+reasons.
+
+## Per-bucket S3Guard configuration
+
+In production, it is likely only some buckets will have S3Guard enabled;
+those which are read-only may have disabled, for example. Equally importantly,
+buckets in different regions should have different tables, each
+in the relevant region.
+
+These options can be managed through S3A's [per-bucket configuration
+mechanism](./index.html#Configuring_different_S3_buckets).
+All options with the under `fs.s3a.bucket.BUCKETNAME.KEY` are propagated
+to the options `fs.s3a.KEY` *for that bucket only*.
+
+As an example, here is a configuration to use different metadata stores
+and tables for different buckets
+
+First, we define shortcuts for the metadata store classnames
+
+
+```xml
+<property>
+  <name>s3guard.null</name>
+  <value>org.apache.hadoop.fs.s3a.s3guard.NullMetadataStore</value>
+</property>
+
+<property>
+  <name>s3guard.dynamo</name>
+  <value>org.apache.hadoop.fs.s3a.s3guard.DynamoDBMetadataStore</value>
+</property>
+```
+
+Next, Amazon's public landsat database is configured with no
+metadata store
+
+```xml
+<property>
+  <name>fs.s3a.bucket.landsat-pds.metadatastore.impl</name>
+  <value>${s3guard.null}</value>
+  <description>The read-only landsat-pds repository isn't
+  managed by S3Guard</description>
+</property>
+```
+
+Next the `ireland-2` and `ireland-offline` buckets are configured with
+DynamoDB as the store, and a shared table `production-table`
+
+
+```xml
+<property>
+  <name>fs.s3a.bucket.ireland-2.metadatastore.impl</name>
+  <value>${s3guard.dynamo}</value>
+</property>
+
+<property>
+  <name>fs.s3a.bucket.ireland-offline.metadatastore.impl</name>
+  <value>${s3guard.dynamo}</value>
+</property>
+
+<property>
+  <name>fs.s3a.bucket.ireland-2.s3guard.ddb.table</name>
+  <value>production-table</value>
+</property>
+```
+
+The region of this table is automatically set to be that of the buckets,
+here `eu-west-1`; the same table name may actually be used in different
+regions.
+
+Together then, this configuration enables the DynamoDB Metadata Store
+for two buckets with a shared table, while disabling it for the public
+bucket.
 
-The existing S3 authentication mechanisms can be used, except for one
-exception. Credentials placed in URIs are not supported for S3Guard.  The
-reason is that providing login details in filesystem URIs is considered
-unsafe and thus deprecated.
 
 ## S3Guard Command Line Interface (CLI)
 
-Note that in some cases an AWS region or s3a:// URI can be provided.
+Note that in some cases an AWS region or `s3a://` URI can be provided.
 
 Metadata store URIs include a scheme that designates the backing store. For
-example (e.g. dynamodb://&lt;table_name&gt;). As documented above, AWS region
-can be inferred if the URI to an existing bucket is provided.
+example (e.g. `dynamodb://table_name`;). As documented above, the
+AWS region can be inferred if the URI to an existing bucket is provided.
 
-### Init
 
-```
+The S3A URI must also be provided for per-bucket configuration options
+to be picked up. That is: when an s3a URL is provided on the command line,
+all its "resolved" per-bucket settings are used to connect to, authenticate
+with and configure the S3Guard table. If no such URL is provided, then
+the base settings are picked up.
+
+
+### Create a table: `s3guard init`
+
+```bash
 hadoop s3guard init -meta URI ( -region REGION | s3a://BUCKET )
 ```
 
@@ -241,44 +368,124 @@ Creates and initializes an empty metadata store.
 A DynamoDB metadata store can be initialized with additional parameters
 pertaining to [Provisioned Throughput](http://docs.aws.amazon.com/amazondynamodb/latest/developerguide/HowItWorks.ProvisionedThroughput.html):
 
-```
+```bash
 [-write PROVISIONED_WRITES] [-read PROVISIONED_READS]
 ```
 
-### Import
+Example 1
+
+```bash
+hadoop s3guard init -meta dynamodb://ireland-team -write 5 -read 10 s3a://ireland-1
+```
+
+Creates a table "ireland-team" with a capacity of 5 for writes, 10 for reads,
+in the same location as the bucket "ireland-1".
+
+
+Example 2
 
+```bash
+hadoop s3guard init -meta dynamodb://ireland-team -region eu-west-1
 ```
+
+Creates a table "ireland-team" in the same region "s3-eu-west-1.amazonaws.com"
+
+
+### Import a bucket: `s3guard import`
+
+```bash
 hadoop s3guard import [-meta URI] s3a://BUCKET
 ```
 
 Pre-populates a metadata store according to the current contents of an S3
-bucket.
+bucket. If the `-meta` option is omitted, the binding information is taken
+from the `core-site.xml` configuration.
 
-### Diff
+Example
 
+```bash
+hadoop s3guard import s3a://ireland-1
 ```
+
+### Audit a table: `s3guard diff`
+
+```bash
 hadoop s3guard diff [-meta URI] s3a://BUCKET
 ```
 
 Lists discrepancies between a metadata store and bucket. Note that depending on
 how S3Guard is used, certain discrepancies are to be expected.
 
-### Destroy
+Example
 
+```bash
+hadoop s3guard diff s3a://ireland-1
 ```
+
+### Delete a table: `s3guard destroy`
+
+
+Deletes a metadata store. With DynamoDB as the store, this means
+the specific DynamoDB table use to store the metadata.
+
+```bash
 hadoop s3guard destroy [-meta URI] ( -region REGION | s3a://BUCKET )
 ```
 
-Deletes a metadata store.
+This *does not* delete the bucket, only the S3Guard table which it is bound
+to.
+
 
-### Prune
+Examples
 
+```bash
+hadoop s3guard destroy s3a://ireland-1
+```
+
+Deletes the table which the bucket ireland-1 is configured to use
+as its MetadataStore.
+
+```bash
+hadoop s3guard destroy -meta dynamodb://ireland-team -region eu-west-1
 ```
+
+
+
+### Clean up a table, `s3guard prune`
+
+Delete all file entries in the MetadataStore table whose object "modification
+time" is older than the specified age.
+
+```bash
 hadoop s3guard prune [-days DAYS] [-hours HOURS] [-minutes MINUTES]
     [-seconds SECONDS] [-m URI] ( -region REGION | s3a://BUCKET )
 ```
 
-Trims metadata for files that are older than the time given. Must supply at least length of time.
+A time value must be supplied.
+
+1. This does not delete the entries in the bucket itself.
+1. The modification time is effectively the creation time of the objects
+in the S3 Bucket.
+1. Even when an S3A URI is supplied, all entries in the table older than
+a specific age are deleted &mdash; even those from other buckets.
+
+Example
+
+```bash
+hadoop s3guard prune -days 7 s3a://ireland-1
+```
+
+Deletes all entries in the S3Guard table for files older than seven days from
+the table associated with `s3a://ireland-1`.
+
+```bash
+hadoop s3guard prune -hours 1 -minutes 30 -meta dynamodb://ireland-team -region eu-west-1
+```
+
+Delete all entries more than 90 minutes old from the table "ireland-team" in
+the region "eu-west-1".
+
+
 
 ## Debugging and Error Handling
 
@@ -287,7 +494,6 @@ middle of an operation, you may end up with your metadata store having state
 that differs from S3.  The S3Guard CLI commands, covered in the CLI section
 above, can be used to diagnose and repair these issues.
 
-
 There are some logs whose log level can be increased to provide more
 information.
 
@@ -298,7 +504,7 @@ log4j.logger.org.apache.hadoop.fs.s3a.s3guard=DEBUG
 # Log all S3A classes
 log4j.logger.org.apache.hadoop.fs.s3a=DEBUG
 
-# Enable debug logging of AWS Dynamo client
+# Enable debug logging of AWS DynamoDB client
 log4j.logger.com.amazonaws.services.dynamodbv2.AmazonDynamoDB
 
 # Log all HTTP requests made; includes S3 interaction. This may
@@ -308,7 +514,7 @@ log4j.logger.com.amazonaws.request=DEBUG
 ```
 
 If all else fails, S3Guard is designed to allow for easy recovery by deleting
-your metadata store data.  In DynamoDB, this can be accomplished by simply
+the metadata store data. In DynamoDB, this can be accomplished by simply
 deleting the table, and allowing S3Guard to recreate it from scratch.  Note
 that S3Guard tracks recent changes to file metadata to implement consistency.
 Deleting the metadata store table will simply result in a period of eventual
@@ -319,10 +525,10 @@ was deleted.
 
 Operations which modify metadata will make changes to S3 first. If, and only
 if, those operations succeed, the equivalent changes will be made to the
-MetadataStore.
+Metadata Store.
 
-These changes to S3 and MetadataStore are not fully-transactional:  If the S3
-operations succeed, and the subsequent MetadataStore updates fail, the S3
+These changes to S3 and Metadata Store are not fully-transactional:  If the S3
+operations succeed, and the subsequent Metadata Store updates fail, the S3
 changes will *not* be rolled back.  In this case, an error message will be
 logged.
 
@@ -351,6 +557,20 @@ in an incompatible manner. The version marker in tables exists to support
 such an option if it ever becomes necessary, by ensuring that all S3Guard
 client can recognise any version mismatch.
 
+### Security
+
+All users of the DynamoDB table must have write access to it. This
+effectively means they must have write access to the entire object store.
+
+There's not been much testing of using a S3Guard Metadata Store
+with a read-only S3 Bucket. It *should* work, provided all users
+have write access to the DynamoDB table. And, as updates to the Metadata Store
+are only made after successful file creation, deletion and rename, the
+store is *unlikely* to get out of sync, it is still something which
+merits more testing before it could be considered reliable.
+
+### Troubleshooting
+
 #### Error: `S3Guard table lacks version marker.`
 
 The table which was intended to be used as a S3guard metadata store
@@ -376,148 +596,15 @@ bucket. Upgrade the application/library.
 If the expected version is higher than the actual version, then the table
 itself will need upgrading.
 
-## Testing S3Guard
-
-The basic strategy for testing S3Guard correctness consists of:
-
-1. MetadataStore Contract tests.
-
-    The MetadataStore contract tests are inspired by the Hadoop FileSystem and
-    FileContext contract tests.  Each implementation of the MetadataStore interface
-    subclasses the `MetadataStoreTestBase` class and customizes it to initialize
-    their MetadataStore.  This test ensures that the different implementations
-    all satisfy the semantics of the MetadataStore API.
-
-2. Running existing S3A unit and integration tests with S3Guard enabled.
-
-    You can run the S3A integration tests on top of S3Guard by configuring your
-    MetadataStore (as documented above) in your
-    `hadoop-tools/hadoop-aws/src/test/resources/core-site.xml` or
-    `hadoop-tools/hadoop-aws/src/test/resources/auth-keys.xml` files.
-    Next run the S3A integration tests as outlined in the *Running the Tests* section
-    of the [S3A documentation](./index.html)
-
-3. Running fault-injection tests that test S3Guard's consistency features.
-
-    The `ITestS3GuardListConsistency` uses failure injection to ensure
-    that list consistency logic is correct even when the underlying storage is
-    eventually consistent.
-
-    The integration test adds a shim above the Amazon S3 Client layer that injects
-    delays in object visibility.
-
-    All of these tests will be run if you follow the steps listed in step 2 above.
-
-    No charges are incurred for using this store, and its consistency
-    guarantees are that of the underlying object store instance. <!-- :) -->
-
-## Testing S3 with S3Guard Enabled
-
-All the S3A tests which work with a private repository can be configured to
-run with S3Guard by using the `s3guard` profile. When set, this will run
-all the tests with local memory for the metadata set to "non-authoritative" mode.
-
-```bash
-mvn -T 1C verify -Dparallel-tests -DtestsThreadCount=6 -Ds3guard 
-```
-
-When the `s3guard` profile is enabled, following profiles can be specified:
-
-* `dynamo`: use an AWS-hosted DynamoDB table; creating the table if it does
-  not exist. You will have to pay the bills for DynamoDB web service.
-* `dynamodblocal`: use an in-memory DynamoDBLocal server instead of real AWS
-  DynamoDB web service; launch the server if it is not yet started; creating the
-  table if it does not exist. You won't be charged bills for using DynamoDB in
-  test. However, the DynamoDBLocal is a simulator of real AWS DynamoDB and is
-  maintained separately, so it may be stale.
-* `non-auth`: treat the s3guard metadata as authorative
-
-```bash
-mvn -T 1C verify -Dparallel-tests -DtestsThreadCount=6 -Ds3guard -Ddynamo -Dauth 
-```
-
-When experimenting with options, it is usually best to run a single test suite
-at a time until the operations appear to be working.
-
-```bash
-mvn -T 1C verify -Dtest=skip -Dit.test=ITestS3AMiscOperations -Ds3guard -Ddynamo
-```
-
-### Notes
-
-1. If the `s3guard` profile is not set, then the s3guard properties are those
-of the test configuration set in `contract-test-options.xml` or `auth-keys.xml`
-
-If the `s3guard` profile *is* set, 
-1. The s3guard options from maven (the dynamo and authoritative flags)
-  overwrite any previously set. in the configuration files.
-1. Dynamo will be configured to create any missing tables.
-
-### Warning About Concurrent Tests
-
-You should not run S3A and S3N tests in parallel on the same bucket.  This is
-especially true when S3Guard is enabled.  S3Guard requires that all clients
-that are modifying the bucket have S3Guard enabled, so having S3N
-integration tests running in parallel with S3A tests will cause strange
-failures.
-
-### Scale Testing MetadataStore Directly
-
-We also have some scale tests that exercise MetadataStore implementations
-directly.  These allow us to ensure were are robust to things like DynamoDB
-throttling, and compare performance for different implementations. See the
-main [S3A documentation](./index.html) for more details on how to enable the
-S3A scale tests.
-
-The two scale tests here are `ITestDynamoDBMetadataStoreScale` and
-`ITestLocalMetadataStoreScale`.  To run the DynamoDB test, you will need to
-define your table name and region in your test configuration.  For example,
-the following settings allow us to run `ITestDynamoDBMetadataStoreScale` with
-artificially low read and write capacity provisioned, so we can judge the
-effects of being throttled by the DynamoDB service:
-
-```
-<property>
-    <name>scale.test.operation.count</name>
-    <value>10</value>
-</property>
-<property>
-    <name>scale.test.directory.count</name>
-    <value>3</value>
-</property>
-<property>
-    <name>fs.s3a.scale.test.enabled</name>
-    <value>true</value>
-</property>
-<property>
-    <name>fs.s3a.s3guard.ddb.table</name>
-    <value>my-scale-test</value>
-</property>
-<property>
-    <name>fs.s3a.s3guard.ddb.region</name>
-    <value>us-west-2</value>
-</property>
-<property>
-    <name>fs.s3a.s3guard.ddb.table.create</name>
-    <value>true</value>
-</property>
-<property>
-    <name>fs.s3a.s3guard.ddb.table.capacity.read</name>
-    <value>10</value>
-</property>
-<property>
-    <name>fs.s3a.s3guard.ddb.table.capacity.write</name>
-    <value>10</value>
-</property>
-```
-
-### Testing only: Local Metadata Store
+#### Error `"DynamoDB table TABLE does not exist in region REGION; auto-creation is turned off"`
 
-There is an in-memory metadata store for testing.
+S3Guard could not find the DynamoDB table for the Metadata Store,
+and it was not configured to create it. Either the table was missing,
+or the configuration is preventing S3Guard from finding the table.
 
-```xml
-<property>
-  <name>fs.s3a.metadatastore.impl</name>
-  <value>org.apache.hadoop.fs.s3a.s3guard.LocalMetadataStore</value>
-</property>
-```
+1. Verify that the value of `fs.s3a.s3guard.ddb.table` is correct.
+1. If the region for an existing table has been set in
+`fs.s3a.s3guard.ddb.region`, verify that the value is correct.
+1. If the region is not set, verify that the table exists in the same
+region as the bucket being used.
+1. Create the table if necessary.

hadoop-tools/hadoop-aws/src/site/markdown/tools/hadoop-aws/testing.md

@@ -821,28 +821,29 @@ using an absolute XInclude reference to it.
 # Failure Injection
 
 **Warning do not enable any type of failure injection in production.  The
-following settings are for test development only.**
-
-## Inconsistency Injection
+following settings are for testing only.**
 
 One of the challenges with S3A integration tests is the fact that S3 is an
 eventually-consistent storage system.  In practice, we rarely see delays in
-visibility of recently created objects both in listings (listStatus()) and
-when getting a single file's metadata (getFileStatus()).  Since this behavior
+visibility of recently created objects both in listings (`listStatus()`) and
+when getting a single file's metadata (`getFileStatus()`). Since this behavior
 is rare and non-deterministic, thorough integration testing is challenging.
 
-To address this, we developed a shim layer on top of the `AmazonS3Client`
+To address this, S3A supports a shim layer on top of the `AmazonS3Client`
 class which artificially delays certain paths from appearing in listings.
 This is implemented in the class `InconsistentAmazonS3Client`.
 
+## Simulating List Inconsistencies
+
 ### Enabling the InconsistentAmazonS3CClient
 
 There are two ways of enabling the `InconsistentAmazonS3Client`: at
-config-time, or programmatically.  For an example of programmatic test usage,
+config-time, or programmatically. For an example of programmatic test usage,
 see `ITestS3GuardListConsistency`.
 
-To enable the inconsistency injecting client via configuration, set the
-following class name for the client factory configuration:
+To enable the fault-injecting client via configuration, switch the
+S3A client to use the "Inconsistent S3 Client Factory" when connecting to
+S3:
 
 ```xml
 <property>
@@ -855,9 +856,9 @@ The inconsistent client works by:
 
 1. Choosing which objects will be "inconsistent" at the time the object is
 created or deleted.
-2. When listObjects is called, any keys that we have marked as
+2. When `listObjects()` is called, any keys that we have marked as
 inconsistent above will not be returned in the results (until the
-configured delay has elapsed).  Similarly, deleted items may be *added* to
+configured delay has elapsed). Similarly, deleted items may be *added* to
 missing results to delay the visibility of the delete.
 
 There are two ways of choosing which keys (filenames) will be affected: By
@@ -876,15 +877,15 @@ substring, and by random probability.
 ```
 
 By default, any object which has the substring "DELAY_LISTING_ME" in its key
-will subject to delayed visibility.  For example, the path
+will subject to delayed visibility. For example, the path
 `s3a://my-bucket/test/DELAY_LISTING_ME/file.txt` would match this condition.
 To match all keys use the value "\*" (a single asterisk). This is a special
 value: *We don't support arbitrary wildcards.*
 
-The default probability of delaying an object is 1.0.  This means that *all*
+The default probability of delaying an object is 1.0. This means that *all*
 keys that match the substring will get delayed visibility. Note that we take
 the logical *and* of the two conditions (substring matches *and* probability
-random chance occurs).  Here are some example configurations:
+random chance occurs). Here are some example configurations:
 
 ```
 | substring | probability |  behavior                                  |
@@ -910,33 +911,189 @@ The default is 5000 milliseconds (five seconds).
 </property>
 ```
 
-#### Limitations of Inconsistency Injection
+Future versions of this client will introduce new failure modes,
+with simulation of S3 throttling exceptions the next feature under
+development.
+
+### Limitations of Inconsistency Injection
 
-Although we can delay visibility of an object or parent directory va the
-`InconsistentAmazonS3Client` we do not keep the key of that object from
-appearing in all prefix searches.  For example, if we create the following
+Although `InconsistentAmazonS3Client` can delay the visibility of an object
+or parent directory, it does not prevent the key of that object from
+appearing in all prefix searches. For example, if we create the following
 object with the default configuration above, in an otherwise empty bucket:
 
 ```
- s3a://bucket/a/b/c/DELAY_LISTING_ME
+s3a://bucket/a/b/c/DELAY_LISTING_ME
 ```
 
-Then the following paths will still be visible as directories:
+Then the following paths will still be visible as directories (ignoring
+possible real-world inconsistencies):
+
+```
+s3a://bucket/a
+s3a://bucket/a/b
+```
+
+Whereas `getFileStatus()` on the following *will* be subject to delayed
+visibility (`FileNotFoundException` until delay has elapsed):
+
+```
+s3a://bucket/a/b/c
+s3a://bucket/a/b/c/DELAY_LISTING_ME
+```
+
+In real-life S3 inconsistency, however, we expect that all the above paths
+(including `a` and `b`) will be subject to delayed visiblity.
+
+### Using the `InconsistentAmazonS3CClient` in downstream integration tests
+
+The inconsistent client is shipped in the `hadoop-aws` JAR, so it can
+be used in applications which work with S3 to see how they handle
+inconsistent directory listings.
+
+## Testing S3Guard
+
+The basic strategy for testing S3Guard correctness consists of:
+
+1. MetadataStore Contract tests.
+
+    The MetadataStore contract tests are inspired by the Hadoop FileSystem and
+    `FileContext` contract tests.  Each implementation of the `MetadataStore` interface
+    subclasses the `MetadataStoreTestBase` class and customizes it to initialize
+    their MetadataStore.  This test ensures that the different implementations
+    all satisfy the semantics of the MetadataStore API.
 
+2. Running existing S3A unit and integration tests with S3Guard enabled.
+
+    You can run the S3A integration tests on top of S3Guard by configuring your
+    `MetadataStore` in your
+    `hadoop-tools/hadoop-aws/src/test/resources/core-site.xml` or
+    `hadoop-tools/hadoop-aws/src/test/resources/auth-keys.xml` files.
+    Next run the S3A integration tests as outlined in the *Running the Tests* section
+    of the [S3A documentation](./index.html)
+
+3. Running fault-injection tests that test S3Guard's consistency features.
+
+    The `ITestS3GuardListConsistency` uses failure injection to ensure
+    that list consistency logic is correct even when the underlying storage is
+    eventually consistent.
+
+    The integration test adds a shim above the Amazon S3 Client layer that injects
+    delays in object visibility.
+
+    All of these tests will be run if you follow the steps listed in step 2 above.
+
+    No charges are incurred for using this store, and its consistency
+    guarantees are that of the underlying object store instance. <!-- :) -->
+
+## Testing S3A with S3Guard Enabled
+
+All the S3A tests which work with a private repository can be configured to
+run with S3Guard by using the `s3guard` profile. When set, this will run
+all the tests with local memory for the metadata set to "non-authoritative" mode.
+
+```bash
+mvn -T 1C verify -Dparallel-tests -DtestsThreadCount=6 -Ds3guard
 ```
- s3a://bucket/a
- s3a://bucket/a/b
+
+When the `s3guard` profile is enabled, following profiles can be specified:
+
+* `dynamo`: use an AWS-hosted DynamoDB table; creating the table if it does
+  not exist. You will have to pay the bills for DynamoDB web service.
+* `dynamodblocal`: use an in-memory DynamoDBLocal server instead of real AWS
+  DynamoDB web service; launch the server and creating the table.
+  You won't be charged bills for using DynamoDB in test. As it runs in-JVM,
+  the table isn't shared across other tests running in parallel.
+* `non-auth`: treat the S3Guard metadata as authorative.
+
+```bash
+mvn -T 1C verify -Dparallel-tests -DtestsThreadCount=6 -Ds3guard -Ddynamo -Dauth
 ```
 
-Whereas getFileStatus() on the following *will* be subject to delayed
-visibility (FileNotFoundException until delay has elapsed):
+When experimenting with options, it is usually best to run a single test suite
+at a time until the operations appear to be working.
 
+```bash
+mvn -T 1C verify -Dtest=skip -Dit.test=ITestS3AMiscOperations -Ds3guard -Ddynamo
 ```
- s3a://bucket/a/b/c
- s3a://bucket/a/b/c/DELAY_LISTING_ME
+
+### Notes
+
+1. If the `s3guard` profile is not set, then the S3Guard properties are those
+of the test configuration set in `contract-test-options.xml` or `auth-keys.xml`
+
+If the `s3guard` profile *is* set,
+1. The S3Guard options from maven (the dynamo and authoritative flags)
+  overwrite any previously set in the configuration files.
+1. DynamoDB will be configured to create any missing tables.
+
+### Warning About Concurrent Tests
+
+You must not run S3A and S3N tests in parallel on the same bucket.  This is
+especially true when S3Guard is enabled.  S3Guard requires that all clients
+that are modifying the bucket have S3Guard enabled, so having S3N
+integration tests running in parallel with S3A tests will cause strange
+failures.
+
+### Scale Testing MetadataStore Directly
+
+There are some scale tests that exercise Metadata Store implementations
+directly. These ensure that S3Guard is are robust to things like DynamoDB
+throttling, and compare performance for different implementations. These
+are included in the scale tests executed when `-Dscale` is passed to
+the maven command line.
+
+The two S3Guard scale testse are `ITestDynamoDBMetadataStoreScale` and
+`ITestLocalMetadataStoreScale`.  To run the DynamoDB test, you will need to
+define your table name and region in your test configuration.  For example,
+the following settings allow us to run `ITestDynamoDBMetadataStoreScale` with
+artificially low read and write capacity provisioned, so we can judge the
+effects of being throttled by the DynamoDB service:
+
+```xml
+<property>
+  <name>scale.test.operation.count</name>
+  <value>10</value>
+</property>
+<property>
+  <name>scale.test.directory.count</name>
+  <value>3</value>
+</property>
+<property>
+  <name>fs.s3a.scale.test.enabled</name>
+  <value>true</value>
+</property>
+<property>
+  <name>fs.s3a.s3guard.ddb.table</name>
+  <value>my-scale-test</value>
+</property>
+<property>
+  <name>fs.s3a.s3guard.ddb.region</name>
+  <value>us-west-2</value>
+</property>
+<property>
+  <name>fs.s3a.s3guard.ddb.table.create</name>
+  <value>true</value>
+</property>
+<property>
+  <name>fs.s3a.s3guard.ddb.table.capacity.read</name>
+  <value>10</value>
+</property>
+<property>
+  <name>fs.s3a.s3guard.ddb.table.capacity.write</name>
+  <value>10</value>
+</property>
 ```
 
- In real-life S3 inconsistency, however, we expect that all the above paths
- (including `a` and `b`) will be subject to delayed visiblity.
+### Testing only: Local Metadata Store
+
+There is an in-memory Metadata Store for testing.
 
+```xml
+<property>
+  <name>fs.s3a.metadatastore.impl</name>
+  <value>org.apache.hadoop.fs.s3a.s3guard.LocalMetadataStore</value>
+</property>
+```
 
+This is not for use in production.

hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/S3ATestConstants.java

@@ -135,7 +135,7 @@ public interface S3ATestConstants {
   String TEST_STS_ENDPOINT = "test.fs.s3a.sts.endpoint";
 
   /**
-   * Various s3guard tests.
+   * Various S3Guard tests.
    */
   String TEST_S3GUARD_PREFIX = "fs.s3a.s3guard.test";
   String TEST_S3GUARD_ENABLED = TEST_S3GUARD_PREFIX + ".enabled";

hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/S3ATestUtils.java

@@ -331,13 +331,17 @@ public final class S3ATestUtils {
 
   /**
    * Test assumption that S3Guard is/is not enabled.
+   * @param shouldBeEnabled should S3Guard be enabled?
+   * @param originalConf configuration to check
+   * @throws URISyntaxException
    */
   public static void assumeS3GuardState(boolean shouldBeEnabled,
       Configuration originalConf) throws URISyntaxException {
     boolean isEnabled = getTestPropertyBool(originalConf, TEST_S3GUARD_ENABLED,
         originalConf.getBoolean(TEST_S3GUARD_ENABLED, false));
-    Assume.assumeThat("Unexpected S3Guard test state: shouldBeEnabled=" +
-        shouldBeEnabled + " and isEnabled =" + isEnabled,
+    Assume.assumeThat("Unexpected S3Guard test state:"
+            + " shouldBeEnabled=" + shouldBeEnabled
+            + " and isEnabled=" + isEnabled,
         shouldBeEnabled, Is.is(isEnabled));
 
     final String fsname = originalConf.getTrimmed(TEST_FS_S3A_NAME);
@@ -346,8 +350,9 @@ public final class S3ATestUtils {
     final Configuration conf = propagateBucketOptions(originalConf, bucket);
     boolean usingNullImpl = S3GUARD_METASTORE_NULL.equals(
         conf.getTrimmed(S3_METADATA_STORE_IMPL, S3GUARD_METASTORE_NULL));
-    Assume.assumeThat("Unexpected S3Guard test state: shouldBeEnabled=" +
-        shouldBeEnabled + " but usingNullImpl=" + usingNullImpl,
+    Assume.assumeThat("Unexpected S3Guard test state:"
+            + " shouldBeEnabled=" + shouldBeEnabled
+            + " but usingNullImpl=" + usingNullImpl,
         shouldBeEnabled, Is.is(!usingNullImpl));
   }
 
@@ -358,7 +363,7 @@ public final class S3ATestUtils {
   public static void maybeEnableS3Guard(Configuration conf) {
     if (getTestPropertyBool(conf, TEST_S3GUARD_ENABLED,
         conf.getBoolean(TEST_S3GUARD_ENABLED, false))) {
-      // s3guard is enabled.
+      // S3Guard is enabled.
       boolean authoritative = getTestPropertyBool(conf,
           TEST_S3GUARD_AUTHORITATIVE,
           conf.getBoolean(TEST_S3GUARD_AUTHORITATIVE, true));
@@ -603,12 +608,32 @@ public final class S3ATestUtils {
   private S3ATestUtils() {
   }
 
+  /**
+   * Verify the core size, block size and timestamp values of a file.
+   * @param status status entry to check
+   * @param size file size
+   * @param blockSize block size
+   * @param modTime modified time
+   */
   public static void verifyFileStatus(FileStatus status, long size,
       long blockSize, long modTime) {
     verifyFileStatus(status, size, 0, modTime, 0, blockSize, null, null, null);
   }
 
-  public static void verifyFileStatus(FileStatus status, long size,
+  /**
+   * Verify the status entry of a file matches that expected.
+   * @param status status entry to check
+   * @param size file size
+   * @param replication replication factor (may be 0)
+   * @param modTime modified time
+   * @param accessTime access time (may be 0)
+   * @param blockSize block size
+   * @param owner owner (may be null)
+   * @param group user group (may be null)
+   * @param permission permission (may be null)
+   */
+  public static void verifyFileStatus(FileStatus status,
+      long size,
       int replication,
       long modTime,
       long accessTime,
@@ -641,6 +666,16 @@ public final class S3ATestUtils {
     }
   }
 
+  /**
+   * Verify the status entry of a directory matches that expected.
+   * @param status status entry to check
+   * @param replication replication factor
+   * @param modTime modified time
+   * @param accessTime access time
+   * @param owner owner
+   * @param group user group
+   * @param permission permission.
+   */
   public static void verifyDirStatus(FileStatus status,
       int replication,
       long modTime,

hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/s3guard/AbstractMSContract.java

@@ -28,6 +28,6 @@ import java.io.IOException;
  */
 public abstract class AbstractMSContract {
 
-  public abstract FileSystem getFileSystem();
+  public abstract FileSystem getFileSystem() throws IOException;
   public abstract MetadataStore getMetadataStore() throws IOException;
-}
+}

hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/s3guard/S3GuardToolTestBase.java → hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/s3guard/AbstractS3GuardToolTestBase.java

@@ -18,67 +18,69 @@
 
 package org.apache.hadoop.fs.s3a.s3guard;
 
+import java.io.IOException;
+import java.util.concurrent.TimeUnit;
+
+import org.junit.Test;
+
 import org.apache.hadoop.conf.Configuration;
 import org.apache.hadoop.fs.Path;
 import org.apache.hadoop.fs.contract.ContractTestUtils;
 import org.apache.hadoop.fs.s3a.AbstractS3ATestBase;
 import org.apache.hadoop.fs.s3a.Constants;
 import org.apache.hadoop.fs.s3a.S3AFileStatus;
-import org.apache.hadoop.fs.s3a.S3AFileSystem;
 import org.apache.hadoop.fs.s3a.S3ATestUtils;
-import org.junit.After;
-import org.junit.Before;
-import org.junit.Test;
-import static org.apache.hadoop.fs.s3a.s3guard.S3GuardTool.SUCCESS;
+import org.apache.hadoop.io.IOUtils;
 
-import java.io.IOException;
-import java.util.concurrent.TimeUnit;
-
-import static org.junit.Assert.assertNotNull;
-import static org.junit.Assert.assertEquals;
+import static org.apache.hadoop.fs.s3a.s3guard.S3GuardTool.SUCCESS;
 
 /**
  * Common functionality for S3GuardTool test cases.
  */
-public abstract class S3GuardToolTestBase extends AbstractS3ATestBase {
+public abstract class AbstractS3GuardToolTestBase extends AbstractS3ATestBase {
 
   protected static final String OWNER = "hdfs";
 
-  private Configuration conf;
   private MetadataStore ms;
-  private S3AFileSystem fs;
 
-  protected Configuration getConf() {
-    return conf;
+  protected static void expectResult(int expected,
+      String message,
+      S3GuardTool tool,
+      String... args) throws Exception {
+    assertEquals(message, expected, tool.run(args));
   }
 
-  protected MetadataStore getMetadataStore() {
-    return ms;
+  protected static void expectSuccess(
+      String message,
+      S3GuardTool tool,
+      String... args) throws Exception {
+    assertEquals(message, SUCCESS, tool.run(args));
   }
 
-  protected S3AFileSystem getFs() {
-    return fs;
+  protected MetadataStore getMetadataStore() {
+    return ms;
   }
 
   protected abstract MetadataStore newMetadataStore();
 
-  @Before
-  public void setUp() throws Exception {
-    conf = new Configuration();
-    fs = S3ATestUtils.createTestFileSystem(conf);
-    S3ATestUtils.assumeS3GuardState(true, getConf());
+  @Override
+  public void setup() throws Exception {
+    super.setup();
+    S3ATestUtils.assumeS3GuardState(true, getConfiguration());
     ms = newMetadataStore();
-    ms.initialize(fs);
+    ms.initialize(getFileSystem());
   }
 
-  @After
-  public void tearDown() {
+  @Override
+  public void teardown() throws Exception {
+    super.teardown();
+    IOUtils.cleanupWithLogger(LOG, ms);
   }
 
   protected void mkdirs(Path path, boolean onS3, boolean onMetadataStore)
       throws IOException {
     if (onS3) {
-      fs.mkdirs(path);
+      getFileSystem().mkdirs(path);
     }
     if (onMetadataStore) {
       S3AFileStatus status = new S3AFileStatus(true, path, OWNER);
@@ -104,26 +106,26 @@ public abstract class S3GuardToolTestBase extends AbstractS3ATestBase {
    * @param onS3 set to true to create the file on S3.
    * @param onMetadataStore set to true to create the file on the
    *                        metadata store.
-   * @throws IOException
+   * @throws IOException IO problem
    */
   protected void createFile(Path path, boolean onS3, boolean onMetadataStore)
       throws IOException {
     if (onS3) {
-      ContractTestUtils.touch(fs, path);
+      ContractTestUtils.touch(getFileSystem(), path);
     }
 
     if (onMetadataStore) {
       S3AFileStatus status = new S3AFileStatus(100L, System.currentTimeMillis(),
-          fs.qualify(path), 512L, "hdfs");
+          getFileSystem().qualify(path), 512L, "hdfs");
       putFile(ms, status);
     }
   }
 
-  private void testPruneCommand(Configuration cmdConf, String[] args)
+  private void testPruneCommand(Configuration cmdConf, String...args)
       throws Exception {
     Path parent = path("prune-cli");
     try {
-      fs.mkdirs(parent);
+      getFileSystem().mkdirs(parent);
 
       S3GuardTool.Prune cmd = new S3GuardTool.Prune(cmdConf);
       cmd.setMetadataStore(ms);
@@ -133,11 +135,11 @@ public abstract class S3GuardToolTestBase extends AbstractS3ATestBase {
       createFile(new Path(parent, "fresh"), true, true);
 
       assertEquals(2, ms.listChildren(parent).getListing().size());
-      assertEquals("Prune command did not exit successfully - see output",
-          SUCCESS, cmd.run(args));
+      expectSuccess("Prune command did not exit successfully - see output", cmd,
+          args);
       assertEquals(1, ms.listChildren(parent).getListing().size());
     } finally {
-      fs.delete(parent, true);
+      getFileSystem().delete(parent, true);
       ms.prune(Long.MAX_VALUE);
     }
   }
@@ -145,15 +147,15 @@ public abstract class S3GuardToolTestBase extends AbstractS3ATestBase {
   @Test
   public void testPruneCommandCLI() throws Exception {
     String testPath = path("testPruneCommandCLI").toString();
-    testPruneCommand(fs.getConf(), new String[]{"prune", "-seconds", "1",
-        testPath});
+    testPruneCommand(getFileSystem().getConf(),
+        "prune", "-seconds", "1", testPath);
   }
 
   @Test
   public void testPruneCommandConf() throws Exception {
-    conf.setLong(Constants.S3GUARD_CLI_PRUNE_AGE,
+    getConfiguration().setLong(Constants.S3GUARD_CLI_PRUNE_AGE,
         TimeUnit.SECONDS.toMillis(1));
     String testPath = path("testPruneCommandConf").toString();
-    testPruneCommand(conf, new String[]{"prune", testPath});
+    testPruneCommand(getConfiguration(), "prune", testPath);
   }
 }

hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/s3guard/DynamoDBLocalClientFactory.java

@@ -48,12 +48,20 @@ import static org.apache.hadoop.fs.s3a.s3guard.DynamoDBClientFactory.DefaultDyna
  * in DynamoDBLocal. This is for testing purpose only.
  *
  * To use this for creating DynamoDB client in tests:
- * 1. As all DynamoDBClientFactory implementations, this should be configured.
- * 2. The singleton DynamoDBLocal server instance is started automatically when
+ * <ol>
+ * <li>
+ *    As all DynamoDBClientFactory implementations, this should be configured.
+ * </li>
+ * <li>
+ *    The singleton DynamoDBLocal server instance is started automatically when
  *    creating the AmazonDynamoDB client for the first time. It still merits to
  *    launch the server before all the tests and fail fast if error happens.
- * 3. The sever can be stopped explicitly, which is not actually needed in tests
- *    as JVM termination will do that.
+ * </li>
+ * <li>
+ *    The server can be stopped explicitly, which is not actually needed in
+ *    tests as JVM termination will do that.
+ * </li>
+ * </ol>
  *
  * @see DefaultDynamoDBClientFactory
  */

hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/s3guard/ITestS3GuardConcurrentOps.java

@@ -18,10 +18,6 @@
 
 package org.apache.hadoop.fs.s3a.s3guard;
 
-import com.amazonaws.services.dynamodbv2.document.DynamoDB;
-import com.amazonaws.services.dynamodbv2.document.Table;
-import com.amazonaws.services.dynamodbv2.model.ResourceNotFoundException;
-
 import java.util.ArrayList;
 import java.util.List;
 import java.util.Random;
@@ -33,17 +29,20 @@ import java.util.concurrent.ThreadFactory;
 import java.util.concurrent.ThreadPoolExecutor;
 import java.util.concurrent.atomic.AtomicInteger;
 
-import org.apache.hadoop.conf.Configuration;
-import org.apache.hadoop.fs.contract.ContractTestUtils;
-import org.apache.hadoop.fs.s3a.AbstractS3ATestBase;
-import org.apache.hadoop.fs.s3a.Constants;
-
-import org.apache.commons.lang3.StringUtils;
+import com.amazonaws.services.dynamodbv2.document.DynamoDB;
+import com.amazonaws.services.dynamodbv2.document.Table;
+import com.amazonaws.services.dynamodbv2.model.ResourceNotFoundException;
 import org.junit.Assume;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.Timeout;
 
+import org.apache.commons.lang3.StringUtils;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.contract.ContractTestUtils;
+import org.apache.hadoop.fs.s3a.AbstractS3ATestBase;
+import org.apache.hadoop.fs.s3a.Constants;
+
 import static org.apache.hadoop.fs.s3a.Constants.S3GUARD_DDB_REGION_KEY;
 
 /**

hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/s3guard/ITestS3GuardToolDynamoDB.java

@@ -18,24 +18,24 @@
 
 package org.apache.hadoop.fs.s3a.s3guard;
 
+import java.io.IOException;
+import java.util.Random;
+import java.util.concurrent.Callable;
+
 import com.amazonaws.services.dynamodbv2.document.DynamoDB;
 import com.amazonaws.services.dynamodbv2.document.Table;
 import com.amazonaws.services.dynamodbv2.model.ResourceNotFoundException;
+import org.junit.Test;
+
 import org.apache.hadoop.fs.s3a.S3AFileSystem;
 import org.apache.hadoop.fs.s3a.s3guard.S3GuardTool.Destroy;
 import org.apache.hadoop.fs.s3a.s3guard.S3GuardTool.Init;
-import org.junit.Test;
-
-import java.io.IOException;
-import java.util.Random;
-
-import static org.apache.hadoop.fs.s3a.s3guard.S3GuardTool.INVALID_ARGUMENT;
-import static org.apache.hadoop.fs.s3a.s3guard.S3GuardTool.SUCCESS;
+import org.apache.hadoop.test.LambdaTestUtils;
 
 /**
  * Test S3Guard related CLI commands against DynamoDB.
  */
-public class ITestS3GuardToolDynamoDB extends S3GuardToolTestBase {
+public class ITestS3GuardToolDynamoDB extends AbstractS3GuardToolTestBase {
 
   @Override
   protected MetadataStore newMetadataStore() {
@@ -58,39 +58,38 @@ public class ITestS3GuardToolDynamoDB extends S3GuardToolTestBase {
 
   @Test
   public void testInvalidRegion() throws Exception {
-    String testTableName = "testInvalidRegion" + new Random().nextInt();
+    final String testTableName = "testInvalidRegion" + new Random().nextInt();
     String testRegion = "invalidRegion";
     // Initialize MetadataStore
-    Init initCmd = new Init(getFs().getConf());
-    try {
-      initCmd.run(new String[]{
-          "init",
-          "-region", testRegion,
-          "-meta", "dynamodb://" + testTableName
-      });
-    } catch (IOException e) {
-      // Expected
-      return;
-    }
-    fail("Use of invalid region did not fail - table may have been " +
-        "created and not cleaned up: " + testTableName);
+    Init initCmd = new Init(getFileSystem().getConf());
+    LambdaTestUtils.intercept(IOException.class,
+        new Callable<String>() {
+          @Override
+          public String call() throws Exception {
+            int res = initCmd.run(new String[]{
+                "init",
+                "-region", testRegion,
+                "-meta", "dynamodb://" + testTableName
+            });
+            return "Use of invalid region did not fail, returning " + res
+                + "- table may have been " +
+                "created and not cleaned up: " + testTableName;
+          }
+        });
   }
 
   @Test
-  public void testDynamoDBInitDestroyCycle() throws IOException,
-      InterruptedException {
+  public void testDynamoDBInitDestroyCycle() throws Exception {
     String testTableName = "testDynamoDBInitDestroy" + new Random().nextInt();
     String testS3Url = path(testTableName).toString();
-    S3AFileSystem fs = getFs();
+    S3AFileSystem fs = getFileSystem();
     DynamoDB db = null;
     try {
       // Initialize MetadataStore
       Init initCmd = new Init(fs.getConf());
-      assertEquals("Init command did not exit successfully - see output",
-          SUCCESS, initCmd.run(new String[]{
-              "init", "-meta", "dynamodb://" + testTableName,
-              testS3Url
-          }));
+      expectSuccess("Init command did not exit successfully - see output",
+          initCmd,
+          "init", "-meta", "dynamodb://" + testTableName, testS3Url);
       // Verify it exists
       MetadataStore ms = getMetadataStore();
       assertTrue("metadata store should be DynamoDBMetadataStore",
@@ -102,18 +101,24 @@ public class ITestS3GuardToolDynamoDB extends S3GuardToolTestBase {
 
       // Destroy MetadataStore
       Destroy destroyCmd = new Destroy(fs.getConf());
-      assertEquals("Destroy command did not exit successfully - see output",
-          SUCCESS, destroyCmd.run(new String[]{
-              "destroy", "-meta", "dynamodb://" + testTableName,
-              testS3Url
-          }));
+
+      expectSuccess("Destroy command did not exit successfully - see output",
+          destroyCmd,
+          "destroy", "-meta", "dynamodb://" + testTableName, testS3Url);
       // Verify it does not exist
       assertFalse(String.format("%s still exists", testTableName),
           exist(db, testTableName));
+
+      // delete again and expect success again
+      expectSuccess("Destroy command did not exit successfully - see output",
+          destroyCmd,
+          "destroy", "-meta", "dynamodb://" + testTableName, testS3Url);
     } catch (ResourceNotFoundException e) {
-      fail(String.format("DynamoDB table %s does not exist", testTableName));
+      throw new AssertionError(
+          String.format("DynamoDB table %s does not exist", testTableName),
+          e);
     } finally {
-      System.out.println("Warning! Table may have not been cleaned up: " +
+      LOG.warn("Table may have not been cleaned up: " +
           testTableName);
       if (db != null) {
         Table table = db.getTable(testTableName);

hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/s3guard/ITestS3GuardToolLocal.java

@@ -18,12 +18,6 @@
 
 package org.apache.hadoop.fs.s3a.s3guard;
 
-import org.apache.hadoop.fs.FSDataOutputStream;
-import org.apache.hadoop.fs.Path;
-import org.apache.hadoop.fs.s3a.S3AFileSystem;
-import org.apache.hadoop.fs.s3a.s3guard.S3GuardTool.Diff;
-import org.junit.Test;
-
 import java.io.BufferedReader;
 import java.io.ByteArrayInputStream;
 import java.io.ByteArrayOutputStream;
@@ -33,14 +27,20 @@ import java.io.PrintStream;
 import java.util.HashSet;
 import java.util.Set;
 
+import org.junit.Test;
+
+import org.apache.hadoop.fs.FSDataOutputStream;
+import org.apache.hadoop.fs.Path;
+import org.apache.hadoop.fs.s3a.S3AFileSystem;
+import org.apache.hadoop.fs.s3a.s3guard.S3GuardTool.Diff;
+
 import static org.apache.hadoop.fs.s3a.s3guard.S3GuardTool.SUCCESS;
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.fail;
+
 
 /**
  * Test S3Guard related CLI commands against a LocalMetadataStore.
  */
-public class ITestS3GuardToolLocal extends S3GuardToolTestBase {
+public class ITestS3GuardToolLocal extends AbstractS3GuardToolTestBase {
 
   @Override
   protected MetadataStore newMetadataStore() {
@@ -48,8 +48,8 @@ public class ITestS3GuardToolLocal extends S3GuardToolTestBase {
   }
 
   @Test
-  public void testImportCommand() throws IOException {
-    S3AFileSystem fs = getFs();
+  public void testImportCommand() throws Exception {
+    S3AFileSystem fs = getFileSystem();
     MetadataStore ms = getMetadataStore();
     Path parent = path("test-import");
     fs.mkdirs(parent);
@@ -67,8 +67,9 @@ public class ITestS3GuardToolLocal extends S3GuardToolTestBase {
     S3GuardTool.Import cmd = new S3GuardTool.Import(fs.getConf());
     cmd.setMetadataStore(ms);
 
-    assertEquals("Import command did not exit successfully - see output",
-        SUCCESS, cmd.run(new String[]{"import", parent.toString()}));
+    expectSuccess("Import command did not exit successfully - see output",
+        cmd,
+        "import", parent.toString());
 
     DirListingMetadata children =
         ms.listChildren(dir);
@@ -81,7 +82,7 @@ public class ITestS3GuardToolLocal extends S3GuardToolTestBase {
 
   @Test
   public void testDiffCommand() throws IOException {
-    S3AFileSystem fs = getFs();
+    S3AFileSystem fs = getFileSystem();
     MetadataStore ms = getMetadataStore();
     Set<Path> filesOnS3 = new HashSet<>(); // files on S3.
     Set<Path> filesOnMS = new HashSet<>(); // files on metadata store.
@@ -114,30 +115,29 @@ public class ITestS3GuardToolLocal extends S3GuardToolTestBase {
     assertEquals("Diff command did not exit successfully - see output", SUCCESS,
         cmd.run(new String[]{"diff", "-meta", "local://metadata",
             testPath.toString()}, out));
+    out.close();
 
     Set<Path> actualOnS3 = new HashSet<>();
     Set<Path> actualOnMS = new HashSet<>();
     boolean duplicates = false;
-    try (ByteArrayInputStream in =
-             new ByteArrayInputStream(buf.toByteArray())) {
-      try (BufferedReader reader =
-               new BufferedReader(new InputStreamReader(in))) {
-        String line;
-        while ((line = reader.readLine()) != null) {
-          String[] fields = line.split("\\s");
-          assertEquals("[" + line + "] does not have enough fields",
-              4, fields.length);
-          String where = fields[0];
-          Path path = new Path(fields[3]);
-          if (Diff.S3_PREFIX.equals(where)) {
-            duplicates = duplicates || actualOnS3.contains(path);
-            actualOnS3.add(path);
-          } else if (Diff.MS_PREFIX.equals(where)) {
-            duplicates = duplicates || actualOnMS.contains(path);
-            actualOnMS.add(path);
-          } else {
-            fail("Unknown prefix: " + where);
-          }
+    try (BufferedReader reader =
+             new BufferedReader(new InputStreamReader(
+                 new ByteArrayInputStream(buf.toByteArray())))) {
+      String line;
+      while ((line = reader.readLine()) != null) {
+        String[] fields = line.split("\\s");
+        assertEquals("[" + line + "] does not have enough fields",
+            4, fields.length);
+        String where = fields[0];
+        Path path = new Path(fields[3]);
+        if (Diff.S3_PREFIX.equals(where)) {
+          duplicates = duplicates || actualOnS3.contains(path);
+          actualOnS3.add(path);
+        } else if (Diff.MS_PREFIX.equals(where)) {
+          duplicates = duplicates || actualOnMS.contains(path);
+          actualOnMS.add(path);
+        } else {
+          fail("Unknown prefix: " + where);
         }
       }
     }

hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/s3guard/MetadataStoreTestBase.java

@@ -18,14 +18,12 @@
 
 package org.apache.hadoop.fs.s3a.s3guard;
 
-import org.apache.hadoop.conf.Configuration;
-import org.apache.hadoop.fs.FileStatus;
-import org.apache.hadoop.fs.Path;
-import org.apache.hadoop.fs.RemoteIterator;
-import org.apache.hadoop.fs.permission.FsPermission;
-import org.apache.hadoop.fs.s3a.S3ATestUtils;
-import org.apache.hadoop.fs.s3a.Tristate;
-import org.apache.hadoop.io.IOUtils;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.HashSet;
+import java.util.Set;
 
 import com.google.common.collect.Sets;
 import org.junit.After;
@@ -36,12 +34,14 @@ import org.junit.Test;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
-import java.io.IOException;
-import java.util.ArrayList;
-import java.util.Arrays;
-import java.util.Collection;
-import java.util.HashSet;
-import java.util.Set;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.FileStatus;
+import org.apache.hadoop.fs.Path;
+import org.apache.hadoop.fs.RemoteIterator;
+import org.apache.hadoop.fs.permission.FsPermission;
+import org.apache.hadoop.fs.s3a.S3ATestUtils;
+import org.apache.hadoop.fs.s3a.Tristate;
+import org.apache.hadoop.io.IOUtils;
 
 /**
  * Main test class for MetadataStore implementations.
@@ -139,7 +139,6 @@ public abstract class MetadataStoreTestBase extends Assert {
    * MetadataStoreListFilesIterator behavior.
    * @param createNodes List of paths to create
    * @param checkNodes List of paths that the iterator should return
-   * @throws IOException
    */
   private void doTestDescendantsIterator(
       Class implementation, String[] createNodes,
@@ -504,13 +503,7 @@ public abstract class MetadataStoreTestBase extends Assert {
       assertListingsEqual(dirMeta.getListing(), "/a1/b1", "/a1/b2");
     }
 
-    // TODO
-    // 1. Add properties query to MetadataStore interface
-    // supportsAuthoritativeDirectories() or something.
-    // 2. Add "isNew" flag to MetadataStore.put(DirListingMetadata)
-    // 3. If #1 is true, assert that directory is still fully cached here.
-    // assertTrue("Created dir is fully cached", dirMeta.isAuthoritative());
-
+    // TODO HADOOP-14756 instrument MetadataStore for asserting & testing
     dirMeta = ms.listChildren(strToPath("/a1/b1"));
     if (!allowMissing() || dirMeta != null) {
       assertListingsEqual(dirMeta.getListing(), "/a1/b1/file1", "/a1/b1/file2",
@@ -599,7 +592,6 @@ public abstract class MetadataStoreTestBase extends Assert {
   /**
    * Test that the MetadataStore differentiates between the same path in two
    * different buckets.
-   * @throws Exception
    */
   @Test
   public void testMultiBucketPaths() throws Exception {
@@ -621,7 +613,7 @@ public abstract class MetadataStoreTestBase extends Assert {
     if (!allowMissing()) {
       ms.delete(new Path(p2));
       meta = ms.get(new Path(p1));
-      assertNotNull("Path should not have been deleted");
+      assertNotNull("Path should not have been deleted", meta);
     }
     ms.delete(new Path(p1));
   }
@@ -722,7 +714,8 @@ public abstract class MetadataStoreTestBase extends Assert {
    */
 
   /** Modifies paths input array and returns it. */
-  private String[] buildPathStrings(String parent, String... paths) {
+  private String[] buildPathStrings(String parent, String... paths)
+      throws IOException {
     for (int i = 0; i < paths.length; i++) {
       Path p = new Path(strToPath(parent), paths[i]);
       paths[i] = p.toString();
@@ -752,7 +745,7 @@ public abstract class MetadataStoreTestBase extends Assert {
   }
 
   private void assertListingsEqual(Collection<PathMetadata> listing,
-      String ...pathStrs) {
+      String ...pathStrs) throws IOException {
     Set<Path> a = new HashSet<>();
     for (PathMetadata meta : listing) {
       a.add(meta.getFileStatus().getPath());
@@ -768,8 +761,8 @@ public abstract class MetadataStoreTestBase extends Assert {
   private void putListStatusFiles(String dirPath, boolean authoritative,
       String... filenames) throws IOException {
     ArrayList<PathMetadata> metas = new ArrayList<>(filenames .length);
-    for (int i = 0; i < filenames.length; i++) {
-      metas.add(new PathMetadata(makeFileStatus(filenames[i], 100)));
+    for (String filename : filenames) {
+      metas.add(new PathMetadata(makeFileStatus(filename, 100)));
     }
     DirListingMetadata dirMeta =
         new DirListingMetadata(strToPath(dirPath), metas, authoritative);
@@ -825,7 +818,7 @@ public abstract class MetadataStoreTestBase extends Assert {
   /**
    * Convenience to create a fully qualified Path from string.
    */
-  Path strToPath(String p) {
+  Path strToPath(String p) throws IOException {
     final Path path = new Path(p);
     assert path.isAbsolute();
     return path.makeQualified(contract.getFileSystem().getUri(), null);

hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/s3guard/TestDirListingMetadata.java

@@ -18,20 +18,20 @@
 
 package org.apache.hadoop.fs.s3a.s3guard;
 
-import static org.hamcrest.CoreMatchers.*;
-import static org.junit.Assert.*;
-
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.List;
 
-import org.apache.hadoop.fs.Path;
-import org.apache.hadoop.fs.s3a.S3AFileStatus;
-
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.ExpectedException;
 
+import org.apache.hadoop.fs.Path;
+import org.apache.hadoop.fs.s3a.S3AFileStatus;
+
+import static org.hamcrest.CoreMatchers.notNullValue;
+import static org.junit.Assert.*;
+
 /**
  * Unit tests of {@link DirListingMetadata}.
  */

hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/s3guard/TestDynamoDBMetadataStore.java

@@ -50,7 +50,6 @@ import org.apache.hadoop.fs.CommonConfigurationKeysPublic;
 import org.apache.hadoop.fs.FileStatus;
 import org.apache.hadoop.fs.FileSystem;
 import org.apache.hadoop.fs.Path;
-import org.apache.hadoop.fs.s3a.Constants;
 import org.apache.hadoop.fs.s3a.MockS3ClientFactory;
 import org.apache.hadoop.fs.s3a.S3AFileStatus;
 import org.apache.hadoop.fs.s3a.S3AFileSystem;
@@ -81,7 +80,7 @@ public class TestDynamoDBMetadataStore extends MetadataStoreTestBase {
       LoggerFactory.getLogger(TestDynamoDBMetadataStore.class);
   private static final String BUCKET = "TestDynamoDBMetadataStore";
   private static final String S3URI =
-      URI.create(Constants.FS_S3A + "://" + BUCKET + "/").toString();
+      URI.create(FS_S3A + "://" + BUCKET + "/").toString();
   public static final PrimaryKey
       VERSION_MARKER_PRIMARY_KEY = createVersionMarkerPrimaryKey(
       DynamoDBMetadataStore.VERSION_MARKER);
@@ -135,9 +134,9 @@ public class TestDynamoDBMetadataStore extends MetadataStoreTestBase {
           S3ClientFactory.class);
       conf.set(CommonConfigurationKeysPublic.FS_DEFAULT_NAME_KEY, S3URI);
       // setting config for creating a DynamoDBClient against local server
-      conf.set(Constants.ACCESS_KEY, "dummy-access-key");
-      conf.set(Constants.SECRET_KEY, "dummy-secret-key");
-      conf.setBoolean(Constants.S3GUARD_DDB_TABLE_CREATE_KEY, true);
+      conf.set(ACCESS_KEY, "dummy-access-key");
+      conf.set(SECRET_KEY, "dummy-secret-key");
+      conf.setBoolean(S3GUARD_DDB_TABLE_CREATE_KEY, true);
       conf.setClass(S3Guard.S3GUARD_DDB_CLIENT_FACTORY_IMPL,
           DynamoDBLocalClientFactory.class, DynamoDBClientFactory.class);
 
@@ -181,7 +180,7 @@ public class TestDynamoDBMetadataStore extends MetadataStoreTestBase {
     return (DynamoDBMetadataStore) getContract().getMetadataStore();
   }
 
-  private S3AFileSystem getFileSystem() {
+  private S3AFileSystem getFileSystem() throws IOException {
     return (S3AFileSystem) getContract().getFileSystem();
   }
 
@@ -194,13 +193,13 @@ public class TestDynamoDBMetadataStore extends MetadataStoreTestBase {
     final String tableName = "testInitializeWithFileSystem";
     final S3AFileSystem s3afs = getFileSystem();
     final Configuration conf = s3afs.getConf();
-    conf.set(Constants.S3GUARD_DDB_TABLE_NAME_KEY, tableName);
+    conf.set(S3GUARD_DDB_TABLE_NAME_KEY, tableName);
     try (DynamoDBMetadataStore ddbms = new DynamoDBMetadataStore()) {
       ddbms.initialize(s3afs);
       verifyTableInitialized(tableName);
       assertNotNull(ddbms.getTable());
       assertEquals(tableName, ddbms.getTable().getTableName());
-      String expectedRegion = conf.get(Constants.S3GUARD_DDB_REGION_KEY,
+      String expectedRegion = conf.get(S3GUARD_DDB_REGION_KEY,
           s3afs.getBucketLocation(tableName));
       assertEquals("DynamoDB table should be in configured region or the same" +
               " region as S3 bucket",
@@ -217,24 +216,24 @@ public class TestDynamoDBMetadataStore extends MetadataStoreTestBase {
   public void testInitializeWithConfiguration() throws IOException {
     final String tableName = "testInitializeWithConfiguration";
     final Configuration conf = getFileSystem().getConf();
-    conf.unset(Constants.S3GUARD_DDB_TABLE_NAME_KEY);
-    String savedRegion = conf.get(Constants.S3GUARD_DDB_REGION_KEY,
+    conf.unset(S3GUARD_DDB_TABLE_NAME_KEY);
+    String savedRegion = conf.get(S3GUARD_DDB_REGION_KEY,
         getFileSystem().getBucketLocation());
-    conf.unset(Constants.S3GUARD_DDB_REGION_KEY);
+    conf.unset(S3GUARD_DDB_REGION_KEY);
     try (DynamoDBMetadataStore ddbms = new DynamoDBMetadataStore()) {
       ddbms.initialize(conf);
       fail("Should have failed because the table name is not set!");
     } catch (IllegalArgumentException ignored) {
     }
     // config table name
-    conf.set(Constants.S3GUARD_DDB_TABLE_NAME_KEY, tableName);
-    try  (DynamoDBMetadataStore ddbms = new DynamoDBMetadataStore()){
+    conf.set(S3GUARD_DDB_TABLE_NAME_KEY, tableName);
+    try (DynamoDBMetadataStore ddbms = new DynamoDBMetadataStore()) {
       ddbms.initialize(conf);
       fail("Should have failed because as the region is not set!");
     } catch (IllegalArgumentException ignored) {
     }
     // config region
-    conf.set(Constants.S3GUARD_DDB_REGION_KEY, savedRegion);
+    conf.set(S3GUARD_DDB_REGION_KEY, savedRegion);
     try (DynamoDBMetadataStore ddbms = new DynamoDBMetadataStore()) {
       ddbms.initialize(conf);
       verifyTableInitialized(tableName);
@@ -348,12 +347,12 @@ public class TestDynamoDBMetadataStore extends MetadataStoreTestBase {
   @Test
   public void testTableVersionRequired() throws Exception {
     Configuration conf = getFileSystem().getConf();
-    int maxRetries = conf.getInt(Constants.S3GUARD_DDB_MAX_RETRIES, Constants
-        .S3GUARD_DDB_MAX_RETRIES_DEFAULT);
-    conf.setInt(Constants.S3GUARD_DDB_MAX_RETRIES, 3);
+    int maxRetries = conf.getInt(S3GUARD_DDB_MAX_RETRIES,
+        S3GUARD_DDB_MAX_RETRIES_DEFAULT);
+    conf.setInt(S3GUARD_DDB_MAX_RETRIES, 3);
 
     final DynamoDBMetadataStore ddbms = createContract(conf).getMetadataStore();
-    String tableName = conf.get(Constants.S3GUARD_DDB_TABLE_NAME_KEY, BUCKET);
+    String tableName = conf.get(S3GUARD_DDB_TABLE_NAME_KEY, BUCKET);
     Table table = verifyTableInitialized(tableName);
     table.deleteItem(VERSION_MARKER_PRIMARY_KEY);
 
@@ -361,7 +360,7 @@ public class TestDynamoDBMetadataStore extends MetadataStoreTestBase {
     intercept(IOException.class, E_NO_VERSION_MARKER,
         () -> ddbms.initTable());
 
-    conf.setInt(Constants.S3GUARD_DDB_MAX_RETRIES, maxRetries);
+    conf.setInt(S3GUARD_DDB_MAX_RETRIES, maxRetries);
   }
 
   /**
@@ -371,16 +370,15 @@ public class TestDynamoDBMetadataStore extends MetadataStoreTestBase {
   @Test
   public void testTableVersionMismatch() throws Exception {
     final DynamoDBMetadataStore ddbms = createContract().getMetadataStore();
-    String tableName = getFileSystem().getConf().get(Constants
-        .S3GUARD_DDB_TABLE_NAME_KEY, BUCKET);
+    String tableName = getFileSystem().getConf()
+        .get(S3GUARD_DDB_TABLE_NAME_KEY, BUCKET);
     Table table = verifyTableInitialized(tableName);
     table.deleteItem(VERSION_MARKER_PRIMARY_KEY);
     Item v200 = createVersionMarker(VERSION_MARKER, 200, 0);
     table.putItem(v200);
 
     // create existing table
-    intercept(IOException.class, E_INCOMPATIBLE_VERSION,
-        () -> ddbms.initTable());
+    intercept(IOException.class, E_INCOMPATIBLE_VERSION, ddbms::initTable);
   }
 
   /**
@@ -392,13 +390,12 @@ public class TestDynamoDBMetadataStore extends MetadataStoreTestBase {
     final String tableName = "testFailNonexistentTable";
     final S3AFileSystem s3afs = getFileSystem();
     final Configuration conf = s3afs.getConf();
-    conf.set(Constants.S3GUARD_DDB_TABLE_NAME_KEY, tableName);
-    conf.unset(Constants.S3GUARD_DDB_TABLE_CREATE_KEY);
-    try {
-      final DynamoDBMetadataStore ddbms = new DynamoDBMetadataStore();
+    conf.set(S3GUARD_DDB_TABLE_NAME_KEY, tableName);
+    conf.unset(S3GUARD_DDB_TABLE_CREATE_KEY);
+    try (DynamoDBMetadataStore ddbms = new DynamoDBMetadataStore()) {
       ddbms.initialize(s3afs);
-      fail("Should have failed as table does not exist and table auto-creation "
-          + "is disabled");
+      fail("Should have failed as table does not exist and table auto-creation"
+          + " is disabled");
     } catch (IOException ignored) {
     }
   }
@@ -425,11 +422,13 @@ public class TestDynamoDBMetadataStore extends MetadataStoreTestBase {
     assertTrue(status.isDirectory());
     // UNKNOWN is always a valid option, but true / false should not contradict
     if (isEmpty) {
-      assertTrue("Should not be marked non-empty",
-          rootMeta.isEmptyDirectory() != Tristate.FALSE);
+      assertNotSame("Should not be marked non-empty",
+          Tristate.FALSE,
+          rootMeta.isEmptyDirectory());
     } else {
-      assertTrue("Should not be marked empty",
-          rootMeta.isEmptyDirectory() != Tristate.TRUE);
+      assertNotSame("Should not be marked empty",
+          Tristate.TRUE,
+          rootMeta.isEmptyDirectory());
     }
   }
 
@@ -437,7 +436,7 @@ public class TestDynamoDBMetadataStore extends MetadataStoreTestBase {
    * Test that when moving nested paths, all its ancestors up to destination
    * root will also be created.
    * Here is the directory tree before move:
-   *
+   * <pre>
    * testMovePopulateAncestors
    * ├── a
    * │   └── b
@@ -448,7 +447,7 @@ public class TestDynamoDBMetadataStore extends MetadataStoreTestBase {
    * └── c
    *     └── d
    *         └── dest
-   *
+   *</pre>
    * As part of rename(a/b/src, d/c/dest), S3A will enumerate the subtree at
    * a/b/src.  This test verifies that after the move, the new subtree at
    * 'dest' is reachable from the root (i.e. c/ and c/d exist in the table.
@@ -525,7 +524,7 @@ public class TestDynamoDBMetadataStore extends MetadataStoreTestBase {
     final String tableName = "testDeleteTable";
     final S3AFileSystem s3afs = getFileSystem();
     final Configuration conf = s3afs.getConf();
-    conf.set(Constants.S3GUARD_DDB_TABLE_NAME_KEY, tableName);
+    conf.set(S3GUARD_DDB_TABLE_NAME_KEY, tableName);
     try (DynamoDBMetadataStore ddbms = new DynamoDBMetadataStore()) {
       ddbms.initialize(s3afs);
       // we can list the empty table

hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/s3guard/TestLocalMetadataStore.java

@@ -18,18 +18,18 @@
 
 package org.apache.hadoop.fs.s3a.s3guard;
 
+import java.io.IOException;
+import java.util.HashMap;
+import java.util.Map;
+
+import org.junit.Test;
+
 import org.apache.hadoop.conf.Configuration;
 import org.apache.hadoop.fs.FileStatus;
 import org.apache.hadoop.fs.FileSystem;
 import org.apache.hadoop.fs.Path;
 import org.apache.hadoop.fs.s3a.S3ATestUtils;
 
-import org.junit.Test;
-
-import java.io.IOException;
-import java.util.HashMap;
-import java.util.Map;
-
 /**
  * MetadataStore unit test for {@link LocalMetadataStore}.
  */

hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/s3guard/TestNullMetadataStore.java

@@ -29,14 +29,9 @@ import java.io.IOException;
 public class TestNullMetadataStore extends MetadataStoreTestBase {
   private static class NullMSContract extends AbstractMSContract {
     @Override
-    public FileSystem getFileSystem() {
+    public FileSystem getFileSystem() throws IOException {
       Configuration config = new Configuration();
-      try {
-        return FileSystem.getLocal(config);
-      } catch (IOException e) {
-        fail("Error creating LocalFileSystem");
-        return null;
-      }
+      return FileSystem.getLocal(config);
     }
 
     @Override

hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/s3guard/TestPathMetadataDynamoDBTranslation.java

@@ -28,26 +28,23 @@ import com.amazonaws.services.dynamodbv2.document.PrimaryKey;
 import com.amazonaws.services.dynamodbv2.model.AttributeDefinition;
 import com.amazonaws.services.dynamodbv2.model.KeySchemaElement;
 import com.google.common.base.Preconditions;
-import org.apache.hadoop.fs.FileStatus;
+import org.junit.Assert;
 import org.junit.BeforeClass;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.Timeout;
 
+import org.apache.hadoop.fs.FileStatus;
 import org.apache.hadoop.fs.Path;
 import org.apache.hadoop.fs.s3a.S3AFileStatus;
 import org.apache.hadoop.security.UserGroupInformation;
+import org.apache.hadoop.test.LambdaTestUtils;
 
 import static com.amazonaws.services.dynamodbv2.model.KeyType.HASH;
 import static com.amazonaws.services.dynamodbv2.model.KeyType.RANGE;
 import static com.amazonaws.services.dynamodbv2.model.ScalarAttributeType.S;
 import static org.hamcrest.CoreMatchers.anyOf;
 import static org.hamcrest.CoreMatchers.is;
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertNotNull;
-import static org.junit.Assert.assertNull;
-import static org.junit.Assert.assertThat;
-import static org.junit.Assert.fail;
 
 import static org.apache.hadoop.fs.s3a.s3guard.PathMetadataDynamoDBTranslation.*;
 import static org.apache.hadoop.fs.s3a.s3guard.DynamoDBMetadataStore.VERSION_MARKER;
@@ -57,7 +54,7 @@ import static org.apache.hadoop.fs.s3a.s3guard.DynamoDBMetadataStore.VERSION;
  * Test the PathMetadataDynamoDBTranslation is able to translate between domain
  * model objects and DynamoDB items.
  */
-public class TestPathMetadataDynamoDBTranslation {
+public class TestPathMetadataDynamoDBTranslation extends Assert {
 
   private static final Path TEST_DIR_PATH = new Path("s3a://test-bucket/myDir");
   private static final Item TEST_DIR_ITEM = new Item();
@@ -151,7 +148,7 @@ public class TestPathMetadataDynamoDBTranslation {
     assertEquals(bSize, status.getBlockSize());
 
     /*
-     * S3AFileStatue#getModificationTime() report the current time, so the
+     * S3AFileStatue#getModificationTime() reports the current time, so the
      * following assertion is failing.
      *
      * long modTime = item.hasAttribute(MOD_TIME) ? item.getLong(MOD_TIME) : 0;
@@ -195,11 +192,8 @@ public class TestPathMetadataDynamoDBTranslation {
 
   @Test
   public void testPathToKey() throws Exception {
-    try {
-      pathToKey(new Path("/"));
-      fail("Root path should have not been mapped to any PrimaryKey");
-    } catch (IllegalArgumentException ignored) {
-    }
+    LambdaTestUtils.intercept(IllegalArgumentException.class,
+        () -> pathToKey(new Path("/")));
     doTestPathToKey(TEST_DIR_PATH);
     doTestPathToKey(TEST_FILE_PATH);
   }

hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/s3guard/TestS3Guard.java

@@ -18,13 +18,14 @@
 
 package org.apache.hadoop.fs.s3a.s3guard;
 
-import org.apache.hadoop.fs.FileStatus;
-import org.apache.hadoop.fs.Path;
+import java.util.Arrays;
+import java.util.List;
+
 import org.junit.Assert;
 import org.junit.Test;
 
-import java.util.Arrays;
-import java.util.List;
+import org.apache.hadoop.fs.FileStatus;
+import org.apache.hadoop.fs.Path;
 
 /**
  * Tests for the {@link S3Guard} utility class.

hadoop-tools/hadoop-aws/src/test/resources/log4j.properties

@@ -26,7 +26,7 @@ log4j.logger.org.apache.hadoop.util.NativeCodeLoader=ERROR
 # Log S3Guard classes
 #log4j.logger.org.apache.hadoop.fs.s3a.s3guard=DEBUG
 
-# Enable debug logging of AWS Dynamo client
+# Enable debug logging of AWS DynamoDB client
 #log4j.logger.com.amazonaws.services.dynamodbv2.AmazonDynamoDB=DEBUG
 
 # Log all HTTP requests made; includes S3 interaction. This may