HADOOP-16697 preparing for review.

* tests for the CLI, command line parsing etc
* tuning CLI based on manual use (mainly logging)

S3GuardTool is now closeable, and will close its FS and MS in close. Without this, our tests can leak FS and MS instances. We just hadn't noticed.

The CLI entry point of S3GuardTool does the right thing and closes the tool; for our tests we have to explicitly do it. This is done in a mixture of try-with-resources and building up a list of tools to close in test tear down. All tests which patch in an existing metastore need to replace it with a null one before the close operation. Are you

Change-Id: I061eb0ece24e84aaffb4f12a3e7c920011146dd6
Testing: new tests are good, doing more regression testing. Can't run -Dlocal for some reason.

Author: steveloughran

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

@@ -3153,12 +3153,12 @@ protected synchronized void stopAllServices() {
     HadoopExecutors.shutdown(unboundedThreadPool, LOG,
         THREAD_POOL_SHUTDOWN_DELAY_SECONDS, TimeUnit.SECONDS);
     unboundedThreadPool = null;
-    closeAutocloseables(LOG, credentials);
     cleanupWithLogger(LOG,
         metadataStore,
         instrumentation,
         delegationTokens.orElse(null),
         signerManager);
+    closeAutocloseables(LOG, credentials);
     delegationTokens = Optional.empty();
     signerManager = null;
     credentials = null;

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

@@ -34,8 +34,11 @@
 import org.apache.hadoop.fs.s3a.impl.StoreContext;
 import org.apache.hadoop.service.launcher.LauncherExitCodes;
 import org.apache.hadoop.util.DurationInfo;
+import org.apache.hadoop.util.ExitCodeProvider;
 import org.apache.hadoop.util.ExitUtil;
 
+import static org.apache.hadoop.service.launcher.LauncherExitCodes.EXIT_NOT_ACCEPTABLE;
+
 /**
  * Audit a directory tree for being authoritative.
  * One aspect of the audit to be aware of: the root directory is
@@ -48,9 +51,9 @@ public class AuthoritativeAuditOperation extends AbstractStoreOperation {
       AuthoritativeAuditOperation.class);
 
   /**
-   * Exception error code when a path is nonauth in the DB: {@value}.
+   * Exception error code when a path is non-auth in the DB}.
    */
-  public static final int ERROR_ENTRY_NOT_AUTH_IN_DDB = 4;
+  public static final int ERROR_ENTRY_NOT_AUTH_IN_DDB = EXIT_NOT_ACCEPTABLE;
 
   /**
    * Exception error code when a path is not configured to be
@@ -70,19 +73,27 @@ public class AuthoritativeAuditOperation extends AbstractStoreOperation {
   /**  require all directories to be authoritative. */
   private final boolean requireAuthoritative;
 
+  /**
+   * Verbose switch.
+   */
+  private final boolean verbose;
+
   /**
    * Constructor.
    * @param storeContext store context.
    * @param metastore metastore
    * @param requireAuthoritative require all directories to be authoritative
+   * @param verbose verbose output
    */
   public AuthoritativeAuditOperation(
       final StoreContext storeContext,
       final DynamoDBMetadataStore metastore,
-      final boolean requireAuthoritative) {
+      final boolean requireAuthoritative,
+      final boolean verbose) {
     super(storeContext);
     this.metastore = metastore;
     this.requireAuthoritative = requireAuthoritative;
+    this.verbose = verbose;
   }
 
   /**
@@ -122,11 +133,6 @@ public Pair<Integer, Integer> audit(Path path) throws IOException {
     try (DurationInfo ignored =
              new DurationInfo(LOG, "audit %s", path)) {
       return executeAudit(path, requireAuthoritative, true);
-    } catch (NonAuthoritativeDirException p) {
-      throw new ExitUtil.ExitException(
-          ERROR_ENTRY_NOT_AUTH_IN_DDB,
-          p.toString(),
-          p);
     }
   }
 
@@ -142,7 +148,8 @@ public Pair<Integer, Integer> audit(Path path) throws IOException {
    * @throws NonAuthoritativeDirException if a non-auth dir was found.
    */
   @VisibleForTesting
-  Pair<Integer, Integer> executeAudit(final Path path,
+  Pair<Integer, Integer> executeAudit(
+      final Path path,
       final boolean requireAuth,
       final boolean recursive) throws IOException {
     int dirs = 0;
@@ -166,16 +173,24 @@ Pair<Integer, Integer> executeAudit(final Path path,
     while (!queue.isEmpty()) {
       dirs++;
       final DDBPathMetadata dir = queue.poll();
-      LOG.info("Directory {} {} authoritative",
-          dir.getFileStatus().getPath(),
-          dir.isAuthoritativeDir() ? "is" : "is not");
+      final Path p = dir.getFileStatus().getPath();
+      // log a message about the dir state, with root treated specially
+      if (!p.isRoot()) {
+        LOG.info("Directory {} {} authoritative",
+            p,
+            dir.isAuthoritativeDir() ? "is" : "is not");
+      } else {
+        // this is done to avoid the confusing message about root not being
+        // authoritative
+        LOG.info("Root directory {}", p);
+      }
       LOG.debug("Directory {}", dir);
       verifyAuthDir(dir, requireAuth);
 
       // list its children
       if (recursive) {
         final DirListingMetadata entry = metastore.listChildren(
-            dir.getFileStatus().getPath());
+            p);
 
         if (entry != null) {
           final Collection<PathMetadata> listing = entry.getListing();
@@ -196,17 +211,42 @@ Pair<Integer, Integer> executeAudit(final Path path,
         }
       }
     }
+    // end of scan
+    if (dirs == 1 && isRoot) {
+      LOG.info("The store has no directories to scan");
+    } else {
+      LOG.info("Scanned {} directories - {} were not marked as authoritative",
+          dirs, nonauth);
+    }
     return Pair.of(dirs, nonauth);
   }
 
   /**
    * A directory was found which was non-authoritative.
+   * The exit code for this operation is
+   * {@link LauncherExitCodes#EXIT_NOT_ACCEPTABLE} -This is what the S3Guard
+   * will return.
    */
-  public static class NonAuthoritativeDirException extends PathIOException {
-
-    public NonAuthoritativeDirException(final Path path) {
+  public static class NonAuthoritativeDirException extends PathIOException
+      implements ExitCodeProvider {
+
+    /**
+     * Instantiate.
+     * @param path the path which is non-authoritative.
+     */
+    private NonAuthoritativeDirException(final Path path) {
       super(path.toString(), E_NONAUTH);
     }
+
+    @Override
+    public int getExitCode() {
+      return ERROR_ENTRY_NOT_AUTH_IN_DDB;
+    }
+
+    @Override
+    public String toString() {
+      return getMessage();
+    }
   }
 
 }

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

@@ -393,7 +393,7 @@ public void initialize(FileSystem fs, ITtlTimeProvider ttlTp)
       throws IOException {
     Preconditions.checkNotNull(fs, "Null filesystem");
     Preconditions.checkArgument(fs instanceof S3AFileSystem,
-        "DynamoDBMetadataStore only supports S3A filesystem.");
+        "DynamoDBMetadataStore only supports S3A filesystem - not %s", fs );
     bindToOwnerFilesystem((S3AFileSystem) fs);
     final String bucket = owner.getBucket();
     String confRegion = conf.getTrimmed(S3GUARD_DDB_REGION_KEY);
@@ -776,7 +776,7 @@ private DDBPathMetadata innerGet(Path path, boolean wantEmptyDirectoryFlag)
 
         // If directory is authoritative, we can set the empty directory flag
         // to TRUE or FALSE. Otherwise FALSE, or UNKNOWN.
-        if(meta.isAuthoritativeDir()) {
+        if (meta.isAuthoritativeDir()) {
           meta.setIsEmptyDirectory(
               hasChildren ? Tristate.FALSE : Tristate.TRUE);
         } else {
@@ -928,7 +928,7 @@ private Collection<DDBPathMetadata> completeAncestry(
               path, entry);
         }
       }
-      // add the entry to the ancestry map as a requested value.
+      // add the entry to the ancestry map as an explicitly requested entry.
       ancestry.put(path, Pair.of(EntryOrigin.Requested, entry));
       Path parent = path.getParent();
       while (!parent.isRoot() && !ancestry.containsKey(parent)) {
@@ -940,7 +940,8 @@ private Collection<DDBPathMetadata> completeAncestry(
           final Item item = getConsistentItem(parent);
           if (item != null && !itemToPathMetadata(item, username).isDeleted()) {
             // This is an undeleted entry found in the database.
-            // register it in ancestor state and map of entries to create
+            // register it in ancestor state and in the map of entries to create
+            // as a retrieved entry
             md = itemToPathMetadata(item, username);
             LOG.debug("Found existing entry for parent: {}", md);
             newEntry = Pair.of(EntryOrigin.Retrieved, md);
@@ -951,6 +952,7 @@ private Collection<DDBPathMetadata> completeAncestry(
             final S3AFileStatus status = makeDirStatus(parent, username);
             md = new DDBPathMetadata(status, Tristate.FALSE,
                 false, false, ttlTimeProvider.getNow());
+            // declare to be a generated entry
             newEntry =  Pair.of(EntryOrigin.Generated, md);
           }
           // insert into the ancestor state to avoid further checks
@@ -961,7 +963,9 @@ private Collection<DDBPathMetadata> completeAncestry(
       }
     }
     // we now have a list of entries which were not in the operation state.
-    // sort in reverse order of existence
+    // Filter out those which were retrieved, to produce a list of those
+    // which must be written to the database.
+    // TODO sort in reverse order of existence
     return ancestry.values().stream()
         .filter(p -> p.getLeft() != EntryOrigin.Retrieved)
         .map(Pair::getRight)
@@ -2076,19 +2080,28 @@ public RenameTracker initiateRenameOperation(
 
   /**
    * Mark the directories instantiated under the destination path
-   * as authoritative.
+   * as authoritative. That is: all entries in the
+   * operationState (which must be an AncestorState instance),
+   * that are under the destination path.
+   *
+   * The database update synchronized on the operationState, so all other
+   * threads trying to update that state will be blocked until completion.
+   *
+   * This operation is only used in import and at the end of a rename,
+   * so this is not considered an issue.
    * @param dest destination path.
    * @param operationState active state.
    * @throws IOException failure.
    * @return the number of directories marked.
    */
   @Override
-  public int markAsAuthoritative(final Path dest,
+  public int markAsAuthoritative(
+      final Path dest,
       final BulkOperationState operationState) throws IOException {
     if (operationState == null) {
       return 0;
     }
-    AncestorState state = (AncestorState)operationState;
+    final AncestorState state = (AncestorState)operationState;
     // only mark paths under the dest as auth
     final String simpleDestKey = pathToParentKey(dest);
     final String destPathKey = simpleDestKey + "/";
@@ -2152,7 +2165,6 @@ private static void logPut(
       String stateStr = AncestorState.stateAsString(state);
       for (Item item : items) {
         boolean tombstone = !itemExists(item);
-
         boolean isDir = hasBoolAttribute(item, IS_DIR, false);
         boolean auth = hasBoolAttribute(item, IS_AUTHORITATIVE, false);
         OPERATIONS_LOG.debug("{} {} {}{}{}",

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

@@ -166,7 +166,7 @@ private long importDir() throws IOException {
         // instead we merge them
         if (!isDirectory) {
           final PathMetadata existingEntry = S3Guard.getWithTtl(ms, path, null,
-              false);
+              false, true);
           if (existingEntry != null) {
             final S3AFileStatus existingStatus = existingEntry.getFileStatus();
             if (existingStatus.isFile()) {
@@ -239,7 +239,7 @@ private void putParentsIfNotPresent(FileStatus f,
       final ITtlTimeProvider timeProvider
           = getFilesystem().getTtlTimeProvider();
       final PathMetadata pmd = S3Guard.getWithTtl(getStore(), parent,
-          timeProvider, false);
+          timeProvider, false, true);
       if (pmd == null || pmd.isDeleted()) {
         S3AFileStatus dir = DynamoDBMetadataStore.makeDirStatus(parent,
             f.getOwner());