HADOOP-17244. S3A directory delete tombstones dir markers prematurely.

* Fix S3A test failures caused by DeleteOperation bugs added
* Extend S3A tests
* Fix FileContext base test so that raised IOEs are always rethrown

regarding change apache#3, I think those IOEs should be thrown first, but
as the existing code does the asserts in finally(), I'm leaving it alone

Change-Id: Ia3159db7760424264d84fa2e38d2cbb452a9854a

Author: steveloughran

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

@@ -21,7 +21,6 @@
 import java.io.File;
 import java.io.FileNotFoundException;
 import java.io.IOException;
-import java.util.Arrays;
 import java.util.EnumSet;
 import java.util.NoSuchElementException;
 import java.util.concurrent.CompletableFuture;
@@ -1323,39 +1322,17 @@ protected void rename(Path src, Path dst, boolean srcExists,
       LOG.debug("Probing source and destination");
       Assert.assertEquals("Source exists", srcExists, exists(fc, src));
       Assert.assertEquals("Destination exists", dstExists, exists(fc, dst));
-      if (!dstExists) {
-        verifyNoListing(fc, dst);
-      }
     } catch (AssertionError e) {
       if (ioe != null && e.getCause() == null) {
         e.initCause(ioe);
       }
       throw e;
-
     }
-  }
-
-  /**
-   * List a path, verify that there are no direct child entries.
-   * @param path path to scan
-   */
-  protected void verifyNoListing(FileContext fc, final Path path)
-      throws IOException {
-    try {
-      intercept(FileNotFoundException.class, () -> {
-        RemoteIterator<FileStatus> st = fc.listStatus(path);
-        if (st.hasNext()) {
-          return st.next().toString();
-        }
-        return "No exception raised";
-      });
-    } catch (IOException e) {
-      throw e;
-    } catch (Exception e) {
-      throw new AssertionError(e);
-
+    if (ioe != null) {
+      throw ioe;
     }
   }
+
   private boolean containsPath(Path path, FileStatus[] filteredPaths)
     throws IOException {
     for(int i = 0; i < filteredPaths.length; i ++) { 

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

@@ -366,7 +366,7 @@ protected void deleteDirectoryTree(final Path path,
           S3AFileStatus next = objects.next();
           LOG.debug("Found Unlisted entry {}", next);
           queueForDeletion(deletionKey(next), null,
-              objects.next().isDirectory());
+              next.isDirectory());
         }
         if (extraFilesDeleted > 0) {
           LOG.debug("Raw S3 Scan found {} extra file(s) to delete",
@@ -548,7 +548,9 @@ private void asyncDeleteAction(
                 undeletedObjects,
                 state,
                 !auditDeletedKeys));
-        deletedObjects.addAll(result.getDeletedObjects());
+        if (result != null) {
+          deletedObjects.addAll(result.getDeletedObjects());
+        }
         // now the dirs
         List<DeleteObjectsRequest.KeyVersion> dirs = keyList.stream()
             .filter(e -> e.isDirMarker)
@@ -564,15 +566,17 @@ private void asyncDeleteAction(
                 undeletedObjects,
                 state,
                 !auditDeletedKeys));
-        deletedObjects.addAll(result.getDeletedObjects());
+        if (result != null) {
+          deletedObjects.addAll(result.getDeletedObjects());
+        }
       }
       if (!pathList.isEmpty()) {
         // delete file paths only. This stops tombstones
         // being added until the final directory cleanup
         // (HADOOP-17244)
         metadataStore.deletePaths(pathList, state);
       }
-      if (auditDeletedKeys && result != null) {
+      if (auditDeletedKeys) {
         // audit the deleted keys
         if (deletedObjects.size() != keyList.size()) {
           // size mismatch

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

@@ -32,6 +32,7 @@
 import org.assertj.core.api.Assertions;
 import org.junit.Test;
 
+import org.apache.hadoop.fs.FileStatus;
 import org.apache.hadoop.fs.Path;
 import org.apache.hadoop.fs.s3a.impl.StatusProbeEnum;
 import org.apache.hadoop.fs.s3a.impl.StoreContext;
@@ -286,4 +287,27 @@ public int read() {
     s3.putObject(putObjectRequest);
   }
 
+  @Test
+  public void testDirMarkerDelete() throws Throwable {
+    S3AFileSystem fs = getFileSystem();
+    assumeFilesystemHasMetadatastore(getFileSystem());
+    Path baseDir = methodPath();
+    Path subFile = new Path(baseDir, "subdir/file.txt");
+    // adds the s3guard entry
+    fs.mkdirs(baseDir);
+    touch(fs, subFile);
+    // PUT a marker
+    createEmptyObject(fs, fs.pathToKey(baseDir) + "/");
+    fs.delete(baseDir, true);
+    assertPathDoesNotExist("Should have been deleted", baseDir);
+
+    // now create the dir again
+    fs.mkdirs(baseDir);
+    FileStatus fileStatus = fs.getFileStatus(baseDir);
+    Assertions.assertThat(fileStatus)
+        .matches(FileStatus::isDirectory, "Not a directory");
+    Assertions.assertThat(fs.listStatus(baseDir))
+        .describedAs("listing of %s", baseDir)
+        .isEmpty();
+  }
 }

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

@@ -23,6 +23,7 @@
 import java.util.Arrays;
 import java.util.Collection;
 
+import org.assertj.core.api.Assertions;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.Parameterized;
@@ -195,14 +196,17 @@ public void testDirMarkersSubdir() throws Throwable {
         withWhenDeleting(FAKE_DIRECTORIES_DELETED,
             directoriesInPath(subDir) - 1));
 
+    int dirDeleteRequests  = 1;
+    int fileDeleteRequests  = 0;
+    int totalDeleteRequests = dirDeleteRequests + fileDeleteRequests;
 
     // now delete the deep tree.
     verifyMetrics(() ->
           fs.delete(parent, true),
 
         // keeping: the parent dir marker needs deletion alongside
         // the subdir one.
-        with(OBJECT_DELETE_REQUESTS, DELETE_MARKER_REQUEST),
+        with(OBJECT_DELETE_REQUESTS, totalDeleteRequests),
         withWhenKeeping(OBJECT_DELETE_OBJECTS, dirsCreated),
 
         // deleting: only the marker at the bottom needs deleting
@@ -211,9 +215,12 @@ public void testDirMarkersSubdir() throws Throwable {
     // followup with list calls to make sure all is clear.
     verifyNoListing(parent);
     verifyNoListing(subDir);
-    verifyNoListing(new Path(parent, "1"));
-    verifyNoListing(new Path(parent, "1/2/"));
-    verifyNoListing(new Path(parent, "1/2/3"));
+    // now reinstate the directory, which in HADOOP-17244 hitting problems
+    fs.mkdirs(parent);
+    FileStatus[] children = fs.listStatus(parent);
+    Assertions.assertThat(children)
+        .describedAs("Children of %s", parent)
+        .isEmpty();
   }
 
   /**