source and test changes

Author: virajjasani

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

@@ -27,6 +27,7 @@
 import static org.apache.hadoop.fs.FileContextTestHelper.*;
 import static org.apache.hadoop.fs.contract.ContractTestUtils.assertIsDirectory;
 import static org.apache.hadoop.fs.contract.ContractTestUtils.assertIsFile;
+import static org.apache.hadoop.test.LambdaTestUtils.intercept;
 
 import org.apache.hadoop.test.GenericTestUtils;
 import org.slf4j.event.Level;
@@ -131,7 +132,7 @@ public void testMkdirsRecursiveWithExistingDir() throws IOException {
   }
 
   @Test
-  public void testMkdirRecursiveWithExistingFile() throws IOException {
+  public void testMkdirRecursiveWithExistingFile() throws Exception {
     Path f = getTestRootPath(fc, "NonExistant3/aDir");
     fc.mkdir(f, FileContext.DEFAULT_PERM, true);
     assertIsDirectory(fc.getFileStatus(f));
@@ -144,13 +145,12 @@ public void testMkdirRecursiveWithExistingFile() throws IOException {
 
     // try creating another folder which conflicts with filePath
     Path dirPath = new Path(filePath, "bDir/cDir");
-    try {
-      fc.mkdir(dirPath, FileContext.DEFAULT_PERM, true);
-      Assert.fail("Mkdir for " + dirPath
-          + MKDIR_FILE_PRESENT_ERROR);
-    } catch(IOException e) {
-      // failed as expected
-    }
+    intercept(
+        IOException.class,
+        null,
+        "Mkdir for " + dirPath + MKDIR_FILE_PRESENT_ERROR,
+        () -> fc.mkdir(dirPath, FileContext.DEFAULT_PERM, true)
+    );
   }
 
   @Test

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

@@ -3769,8 +3769,7 @@ public boolean mkdirs(Path p, FsPermission permission) throws IOException,
             createStoreContext(),
             path,
             createMkdirOperationCallbacks(),
-            isMagicCommitPath(path),
-            performanceCreation));
+            performanceCreation || isMagicCommitPath(path)));
   }
 
   /**
@@ -4209,7 +4208,6 @@ public boolean createEmptyDir(Path path, StoreContext storeContext)
               storeContext,
               path,
               createMkdirOperationCallbacks(),
-              false,
               performanceCreation));
     }
   }

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

@@ -70,21 +70,22 @@ public class MkdirOperation extends ExecutingStoreOperation<Boolean> {
   private final boolean performanceCreation;
 
   /**
-   * Should checks for ancestors existing be skipped?
-   * This flag is set when working with magic directories.
+   * Initialize Mkdir Operation context for S3A.
+   *
+   * @param storeContext Store context.
+   * @param dir Dir path of the directory.
+   * @param callbacks MkdirCallbacks object used by the Mkdir operation.
+   * @param performanceCreation If true, skip validation of the parent directory
+   * structure.
    */
-  private final boolean isMagicPath;
-
   public MkdirOperation(
       final StoreContext storeContext,
       final Path dir,
       final MkdirCallbacks callbacks,
-      final boolean isMagicPath,
       final boolean performanceCreation) {
     super(storeContext);
     this.dir = dir;
     this.callbacks = callbacks;
-    this.isMagicPath = isMagicPath;
     this.performanceCreation = performanceCreation;
   }
 
@@ -106,14 +107,13 @@ public Boolean execute() throws IOException {
     }
 
     // get the file status of the path.
-    // this is done even for a magic path, to avoid always  issuing PUT
-    // requests. Doing that without a check wouild seem to be an
-    // optimization, but it is not because
-    // 1. PUT is slower than HEAD
-    // 2. Write capacity is less than read capacity on a shard
-    // 3. It adds needless entries in versioned buckets, slowing
-    //    down subsequent operations.
-    FileStatus fileStatus = getPathStatusExpectingDir(dir);
+    // this is not done for magic path i.e. performanceCreation mode.
+    // For performanceCreation mode, we would probe for HEAD only.
+    // For non-performance or regular mode, the probe for both HEAD and LIST would
+    // be done.
+    S3AFileStatus fileStatus = performanceCreation
+        ? probePathStatusOrNull(dir, StatusProbeEnum.HEAD_ONLY)
+        : getPathStatusExpectingDir(dir);
     if (fileStatus != null) {
       if (fileStatus.isDirectory()) {
         return true;
@@ -123,15 +123,6 @@ public Boolean execute() throws IOException {
     }
     // file status was null
 
-    // is the path magic?
-    // If so, we declare success without looking any further
-    if (isMagicPath) {
-      // Create the marker file immediately,
-      // and don't delete markers
-      callbacks.createFakeDirectory(dir, true);
-      return true;
-    }
-
     // if performance creation mode is set, no need to check
     // whether the closest ancestor is dir.
     if (!performanceCreation) {
@@ -143,7 +134,8 @@ public Boolean execute() throws IOException {
 
     // Create the marker file, delete the parent entries
     // if the filesystem isn't configured to retain them
-    callbacks.createFakeDirectory(dir, false);
+    callbacks.createFakeDirectory(dir,
+        performanceCreation);
     return true;
   }
 
@@ -220,7 +212,7 @@ private S3AFileStatus getPathStatusExpectingDir(final Path path)
       throws IOException {
     S3AFileStatus status = probePathStatusOrNull(path,
         StatusProbeEnum.DIRECTORIES);
-    if (status == null && !isMagicPath) {
+    if (status == null && !performanceCreation) {
       status = probePathStatusOrNull(path,
           StatusProbeEnum.FILE);
     }

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

@@ -18,7 +18,9 @@
 
 package org.apache.hadoop.fs.contract.s3a;
 
-import org.assertj.core.api.Assertions;
+import java.io.IOException;
+import java.io.UncheckedIOException;
+
 import org.junit.Test;
 
 import org.apache.hadoop.conf.Configuration;
@@ -27,6 +29,7 @@
 
 import static org.apache.hadoop.fs.s3a.Constants.FS_S3A_CREATE_PERFORMANCE;
 import static org.apache.hadoop.fs.s3a.S3ATestUtils.removeBaseAndBucketOverrides;
+import static org.apache.hadoop.test.LambdaTestUtils.intercept;
 
 /**
  * Test mkdir operations on S3A with create performance mode.
@@ -51,18 +54,21 @@ protected AbstractFSContract createContract(Configuration conf) {
 
   @Test
   public void testMkdirOverParentFile() throws Throwable {
+    intercept(
+        UncheckedIOException.class,
+        MKDIRS_NOT_FAILED_OVER_FILE,
+        "Dir creation should not have failed. "
+            + "Creation performance mode is expected "
+            + "to create dir without checking file "
+            + "status of parent dir.",
+        this::callTestMkdirOverParentFile);
+  }
+
+  private void callTestMkdirOverParentFile() {
     try {
       super.testMkdirOverParentFile();
-      throw new RuntimeException(
-          "Dir creation should not have failed. "
-              + "Creation performance mode is expected "
-              + "to create dir without checking file "
-              + "status of parent dir.");
-    } catch (AssertionError e) {
-      Assertions
-          .assertThat(e)
-          .describedAs("assertion error from testMkdirOverParentFile")
-          .hasMessageStartingWith(MKDIRS_NOT_FAILED_OVER_FILE);
+    } catch (Throwable e) {
+      throw new UncheckedIOException(new IOException(e));
     }
   }
 

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

@@ -25,6 +25,7 @@
 
 import static org.apache.hadoop.fs.s3a.Constants.FS_S3A_CREATE_PERFORMANCE;
 import static org.apache.hadoop.fs.s3a.S3ATestUtils.removeBaseAndBucketOverrides;
+import static org.apache.hadoop.test.LambdaTestUtils.intercept;
 
 /**
  * Extends FileContextCreateMkdirBaseTest for a S3a FileContext with
@@ -53,20 +54,15 @@ public void tearDown() throws Exception {
   }
 
   @Test
-  public void testMkdirRecursiveWithExistingFile() throws IOException {
-    try {
-      super.testMkdirRecursiveWithExistingFile();
-      throw new RuntimeException(
-          "Dir creation should not have failed. "
-              + "Creation performance mode is expected "
-              + "to create dir without checking file "
-              + "status of parent dir.");
-    } catch (AssertionError e) {
-      Assertions
-          .assertThat(e)
-          .describedAs("assertion error from testMkdirRecursiveWithExistingFile")
-          .hasMessageContaining(MKDIR_FILE_PRESENT_ERROR);
-    }
+  public void testMkdirRecursiveWithExistingFile() throws Exception {
+    intercept(
+        AssertionError.class,
+        MKDIR_FILE_PRESENT_ERROR,
+        "Dir creation should not have failed. "
+            + "Creation performance mode is expected "
+            + "to create dir without checking file "
+            + "status of parent dir.",
+        super::testMkdirRecursiveWithExistingFile);
   }
 
 }