HADOOP-17244: multi object delete failure handling

-improve test, fix production code
-transient failure of a root test, likely cause: debugging test runs
had got store/ddb inconsistent. Slightly improved error message
on assert and temp dirs to include full timestamp to identify when
the failure happened.

TODO: ITest of dir-under-dir where a parent dir marker is deleted
but a child dir is not. S3Guard must still be valid

Change-Id: I2c91aa234d333072b2bf7ee9d8d844ebc7d31eb7

Author: steveloughran

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

@@ -254,7 +254,7 @@ public void testRecursiveRootListing() throws IOException {
         fs.listFiles(root, true));
     describe("verifying consistency with treewalk's files");
     ContractTestUtils.TreeScanResults treeWalk = treeWalk(fs, root);
-    treeWalk.assertFieldsEquivalent("files", listing,
+    treeWalk.assertFieldsEquivalent("treewalk vs listFiles(/, true)", listing,
         treeWalk.getFiles(),
         listing.getFiles());
   }

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

@@ -331,7 +331,7 @@ static List<KeyPath> removeUndeletedPaths(
     List<KeyPath> undeleted = extractUndeletedKeyPaths(deleteException, qualifier);
     // and remove them from the undeleted list, matching on key
     for (KeyPath undel : undeleted) {
-      pathsBeingDeleted.removeIf(kp -> kp.equals(undel));
+      pathsBeingDeleted.removeIf(kp -> kp.getPath().equals(undel.getPath()));
     }
     return undeleted;
   }

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

@@ -352,16 +352,17 @@ protected Configuration createConfiguration() {
 
   /**
    * Create a unique path, which includes method name,
-   * multidelete flag and a random UUID.
+   * multidelete flag and a timestamp.
    * @return a string to use for paths.
    * @throws IOException path creation failure.
    */
   private Path uniquePath() throws IOException {
+    long now = System.currentTimeMillis();
     return path(
-        String.format("%s-%s-%04d",
+        String.format("%s-%s-%06d.%03d",
             getMethodName(),
             multiDelete ? "multi" : "single",
-            System.currentTimeMillis() % 10000));
+            now / 1000, now % 1000));
   }
 
   /**
@@ -642,8 +643,6 @@ public void testPartialDirDelete() throws Throwable {
         fileCount, dirCount,
         new ArrayList<>(fileCount),
         dirs);
-        createFiles(fs, readOnlyDir,
-        dirDepth, fileCount, dirCount);
     List<Path> deletableFiles = createFiles(fs,
         writableDir, dirDepth, fileCount, dirCount);
 
@@ -724,6 +723,25 @@ public void testPartialDirDelete() throws Throwable {
         .containsExactlyInAnyOrderElementsOf(readOnlyFiles);
   }
 
+  /**
+   * Verifies the logic of handling directory markers in
+   * delete operations, specifically:
+   * <ol>
+   *   <li>all markers above empty directories MUST be deleted</li>
+   *   <li>all markers above non-empty directories MUST NOT be deleted</li>
+   * </ol>
+   * As the delete list may include subdirectories, we need to work up from
+   * the bottom of the list of deleted files before probing the parents,
+   * that being done by a s3guard get(path, need-empty-directory) call.
+   * <p></p>
+   * This is pretty sensitive code.
+   */
+  @Test
+  public void testSubdirDeleteFailures() throws Throwable {
+    describe("Multiobject delete handling of directorYesFory markers");
+    assume("Multiobject delete only", multiDelete);
+  }
+
   /**
    * Expect the delete() call to fail.
    * @param path path to delete.

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

@@ -137,7 +137,9 @@ private MultiObjectDeleteException createDeleteException(
   }
 
   /**
-   * From a list of paths, build up the list of keys for a delete request.
+   * From a list of paths, build up the list of KeyVersion records
+   * for a delete request.
+   * All the entries will be files (i.e. no trailing /)
    * @param paths path list
    * @return a key list suitable for a delete request.
    */
@@ -152,23 +154,40 @@ public static List<DeleteObjectsRequest.KeyVersion> keysToDelete(
         .collect(Collectors.toList());
   }
 
+  /**
+   * From a list of keys, build up the list of keys for a delete request.
+   * If a key has a trailing /, that will be retained, so it will be
+   * considered a directory during multi-object delete failure handling
+   * @param keys key list
+   * @return a key list suitable for a delete request.
+   */
+  public static List<DeleteObjectsRequest.KeyVersion> toDeleteRequests(
+      List<String> keys) {
+    return keys.stream()
+        .map(DeleteObjectsRequest.KeyVersion::new)
+        .collect(Collectors.toList());
+  }
+
   /**
    * Verify that on a partial delete, the S3Guard tables are updated
    * with deleted items. And only them.
    */
   @Test
   public void testProcessDeleteFailure() throws Throwable {
-    Path pathA = qualifyKey("/a");
-    Path pathAB = qualifyKey("/a/b");
-    Path pathAC = qualifyKey("/a/c");
+    String keyA = "/a/";
+    String keyAB = "/a/b";
+    String keyAC = "/a/c";
+    Path pathA = qualifyKey(keyA);
+    Path pathAB = qualifyKey(keyAB);
+    Path pathAC = qualifyKey(keyAC);
+    List<String> srcKeys = Lists.newArrayList(keyA, keyAB, keyAC);
     List<Path> src = Lists.newArrayList(pathA, pathAB, pathAC);
-    List<DeleteObjectsRequest.KeyVersion> keyList = keysToDelete(src);
+    List<DeleteObjectsRequest.KeyVersion> keyList = toDeleteRequests(srcKeys);
     List<Path> deleteForbidden = Lists.newArrayList(pathAB);
     final List<Path> deleteAllowed = Lists.newArrayList(pathA, pathAC);
-    List<MultiObjectDeleteSupport.KeyPath> forbiddenKP = deleteForbidden.stream()
-        .map(p ->
-            new MultiObjectDeleteSupport.KeyPath(toKey(p), p, false))
-        .collect(Collectors.toList());
+    List<MultiObjectDeleteSupport.KeyPath> forbiddenKP =
+        Lists.newArrayList(
+            new MultiObjectDeleteSupport.KeyPath(keyAB, pathAB, true));
     MultiObjectDeleteException ex = createDeleteException(ACCESS_DENIED,
         forbiddenKP);
     S3ATestUtils.OperationTrackingStore store
@@ -196,7 +215,10 @@ public void testProcessDeleteFailure() throws Throwable {
     // because dir marker retention is on, we expect at least one retained marker
     Assertions.assertThat(retainedMarkers).
         as("Retained Markers")
-        .isNotEmpty();
+        .containsExactly(pathA);
+    Assertions.assertThat(store.getDeleted()).
+        as("List of tombstoned records")
+        .doesNotContain(pathA);
   }