Fixing and adding additional testcase

snvijaya · snvijaya · commit 6ff28f13ded5 · 2020-09-14T14:27:05.000+05:30
diff --git a/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsClient.java b/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsClient.java
@@ -324,7 +324,7 @@ public AbfsRestOperation createPath(final String path,
         try {
           op = getPathStatus(path, false);
         } catch (AbfsRestOperationException ex) {
-          if (e.getStatusCode() == HttpURLConnection.HTTP_NOT_FOUND) {
+          if (ex.getStatusCode() == HttpURLConnection.HTTP_NOT_FOUND) {
             // Is a parallel access case, as file which was found to be
             // present went missing by this request.
             throw new ConcurrentWriteOperationDetectedException(
@@ -342,7 +342,7 @@ public AbfsRestOperation createPath(final String path,
           op = createPathImpl(path, abfsUriQueryBuilder, true, permission,
               umask, eTag);
         } catch (AbfsRestOperationException ex) {
-          if (e.getStatusCode() == HttpURLConnection.HTTP_PRECON_FAILED) {
+          if (ex.getStatusCode() == HttpURLConnection.HTTP_PRECON_FAILED) {
             // Is a parallel access case, as file with eTag was just queried
             // and precondition failure can happen only when another file with
             // different etag got created.
@@ -361,7 +361,8 @@ public AbfsRestOperation createPath(final String path,
     return op;
   }
 
-  private AbfsRestOperation createPathImpl(final String path,
+  @VisibleForTesting
+  public AbfsRestOperation createPathImpl(final String path,
       AbfsUriQueryBuilder abfsUriQueryBuilder,
       final boolean overwrite,
       final String permission,
diff --git a/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAbfsNetworkStatistics.java b/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAbfsNetworkStatistics.java
@@ -110,15 +110,16 @@ public void testAbfsHttpSendStatistics() throws IOException {
     connectionsMade++;
     requestsSent++;
 
+
     try (AbfsOutputStream out = createAbfsOutputStreamWithFlushEnabled(fs,
         sendRequestPath)) {
-      boolean createOverwriteNeedsAdditionalRequest = false;
-      if (fs.getAbfsStore()
-          .getAbfsConfiguration()
-          .isDefaultCreateOverwriteDisabled()) {
-        // if the default overwrite=true behaviour is disabled by config,
-        // the re-create here will need 2 requests.
-        createOverwriteNeedsAdditionalRequest = true;
+
+      // Is a file overwrite case
+      long createRequestCalls = 1;
+      long createTriggeredGFSForETag = 0;
+      if (this.getConfiguration().isDefaultCreateOverwriteDisabled()) {
+        createRequestCalls += 1;
+        createTriggeredGFSForETag = 1;
       }
 
       for (int i = 0; i < LARGE_OPERATIONS; i++) {
@@ -149,22 +150,20 @@ public void testAbfsHttpSendStatistics() throws IOException {
        * wrote each time).
        *
        */
-      long expectedConnectionsMade = connectionsMade + 1
-          + (createOverwriteNeedsAdditionalRequest ? 1 : 0);
-      long expectedSendRequests = requestsSent + 1
-          + (createOverwriteNeedsAdditionalRequest ? 1 : 0);
 
+      connectionsMade += createRequestCalls + createTriggeredGFSForETag;
+      requestsSent += createRequestCalls;
       if (fs.getAbfsStore().isAppendBlobKey(fs.makeQualified(sendRequestPath).toString())) {
         // no network calls are made for hflush in case of appendblob
         assertAbfsStatistics(CONNECTIONS_MADE,
-            expectedConnectionsMade + LARGE_OPERATIONS, metricMap);
+            connectionsMade + LARGE_OPERATIONS, metricMap);
         assertAbfsStatistics(SEND_REQUESTS,
-            expectedSendRequests + LARGE_OPERATIONS, metricMap);
+            requestsSent + LARGE_OPERATIONS, metricMap);
       } else {
         assertAbfsStatistics(CONNECTIONS_MADE,
-            expectedConnectionsMade + LARGE_OPERATIONS * 2, metricMap);
+            connectionsMade + LARGE_OPERATIONS * 2, metricMap);
         assertAbfsStatistics(SEND_REQUESTS,
-            expectedSendRequests + LARGE_OPERATIONS * 2, metricMap);
+            requestsSent + LARGE_OPERATIONS * 2, metricMap);
       }
       assertAbfsStatistics(AbfsStatistic.BYTES_SENT,
           bytesSent + LARGE_OPERATIONS * (testNetworkStatsString.getBytes().length),
@@ -250,14 +249,21 @@ public void testAbfsHttpResponseStatistics() throws IOException {
     try {
 
       /*
-       * Creating a file and writing buffer into it. Also recording the
-       * buffer for future read() call.
+       * Creating a file and writing buffer into it.
+       * This is a file recreate, so it will trigger
+       * 2 extra calls if create overwrite is off by default.
+       * Also recording the buffer for future read() call.
        * This creating outputStream and writing requires 2 *
        * (LARGE_OPERATIONS) get requests.
        */
       StringBuilder largeBuffer = new StringBuilder();
       out = fs.create(getResponsePath);
 
+      long createRequestCalls = 1;
+      if (this.getConfiguration().isDefaultCreateOverwriteDisabled()) {
+        createRequestCalls += 2;
+      }
+
       for (int i = 0; i < LARGE_OPERATIONS; i++) {
         out.write(testResponseString.getBytes());
         out.hflush();
@@ -282,7 +288,8 @@ public void testAbfsHttpResponseStatistics() throws IOException {
        *
        * get_response : get_responses(Last assertion) + 1
        * (OutputStream) + 2 * LARGE_OPERATIONS(Writing and flushing
-       * LARGE_OPERATIONS times) + 1(open()) + 1(read()).
+       * LARGE_OPERATIONS times) + 1(open()) + 1(read()) +
+       * 1 (createOverwriteTriggeredGetForeTag).
        *
        * bytes_received : bytes_received(Last assertion) + LARGE_OPERATIONS *
        * bytes wrote each time (bytes_received is equal to bytes wrote in the
@@ -298,7 +305,8 @@ public void testAbfsHttpResponseStatistics() throws IOException {
             getResponses + 3 + LARGE_OPERATIONS, metricMap);
       } else {
         assertAbfsStatistics(AbfsStatistic.GET_RESPONSES,
-            getResponses + 3 + 2 * LARGE_OPERATIONS, metricMap);
+            getResponses + 2 + createRequestCalls + 2 * LARGE_OPERATIONS,
+            metricMap);
       }
 
     } finally {
@@ -333,4 +341,4 @@ public void testAbfsHttpResponseFailure() throws IOException {
       IOUtils.cleanupWithLogger(LOG, out);
     }
   }
-}
+}
diff --git a/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemCreate.java b/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemCreate.java
@@ -35,9 +35,26 @@
 import org.apache.hadoop.fs.permission.FsPermission;
 import org.apache.hadoop.test.GenericTestUtils;
 
+import org.apache.hadoop.fs.azurebfs.contracts.exceptions.AbfsRestOperationException;
+import org.apache.hadoop.fs.azurebfs.contracts.exceptions.ConcurrentWriteOperationDetectedException;
+import org.apache.hadoop.fs.azurebfs.services.AbfsHttpOperation;
+import org.apache.hadoop.fs.azurebfs.services.AbfsRestOperation;
+import org.apache.hadoop.fs.azurebfs.services.AbfsUriQueryBuilder;
+
+import static java.net.HttpURLConnection.HTTP_CONFLICT;
+import static java.net.HttpURLConnection.HTTP_INTERNAL_ERROR;
+import static java.net.HttpURLConnection.HTTP_NOT_FOUND;
+import static java.net.HttpURLConnection.HTTP_OK;
+import static java.net.HttpURLConnection.HTTP_PRECON_FAILED;
+
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.doThrow;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
 import static org.apache.hadoop.fs.contract.ContractTestUtils.assertIsFile;
 import static org.apache.hadoop.test.LambdaTestUtils.intercept;
-
 import static org.apache.hadoop.fs.azurebfs.AbfsStatistic.CONNECTIONS_MADE;
 
 /**
@@ -286,4 +303,151 @@ public void testCreateFileOverwrite(boolean defaultDisableCreateOverwrite)
         totalConnectionMadeBeforeTest + createRequestCount,
         fs.getInstrumentationMap());
   }
+
+  /**
+   * Test negative scenarios with Create overwrite=false as default
+   * With create overwrite=true ending in 3 calls:
+   * A. Create overwrite=false
+   * B. GFS
+   * C. Create overwrite=true
+   *
+   * Scn1: A fails with HTTP409, leading to B which fails with HTTP404,
+   *        detect parallel access
+   * Scn2: A fails with HTTP409, leading to B which fails with HTTP500,
+   *        fail create with HTTP500
+   * Scn3: A fails with HTTP409, leading to B and then C,
+   *        which fails with HTTP412, detect parallel access
+   * Scn4: A fails with HTTP409, leading to B and then C,
+   *        which fails with HTTP500, fail create with HTTP500
+   * Scn5: A fails with HTTP500, fail create with HTTP500
+   */
+  @Test
+  public void testNegativeScenariosForCreateOverwriteDisabled()
+      throws Throwable {
+
+    final AzureBlobFileSystem currentFs = getFileSystem();
+    Configuration config = new Configuration(this.getRawConfiguration());
+    config.set("fs.azure.disable.default.create.overwrite",
+        Boolean.toString(true));
+
+    final AzureBlobFileSystem fs =
+        (AzureBlobFileSystem) FileSystem.newInstance(currentFs.getUri(),
+            config);
+
+    // Get mock AbfsClient with current config
+    org.apache.hadoop.fs.azurebfs.services.AbfsClient
+        mockClient
+        = org.apache.hadoop.fs.azurebfs.services.TestAbfsClient.getMockAbfsClient(
+        fs.getAbfsStore().getClient(),
+        fs.getAbfsStore().getAbfsConfiguration());
+
+    AbfsRestOperation successOp = mock(
+        AbfsRestOperation.class);
+    AbfsHttpOperation http200Op = mock(
+        AbfsHttpOperation.class);
+    when(http200Op.getStatusCode()).thenReturn(HTTP_OK);
+    when(successOp.getResult()).thenReturn(http200Op);
+
+    AbfsRestOperationException conflictResponseEx
+        = getMockAbfsRestOperationException(HTTP_CONFLICT);
+    AbfsRestOperationException serverErrorResponseEx
+        = getMockAbfsRestOperationException(HTTP_INTERNAL_ERROR);
+    AbfsRestOperationException fileNotFoundResponseEx
+        = getMockAbfsRestOperationException(HTTP_NOT_FOUND);
+    AbfsRestOperationException preConditionResponseEx
+        = getMockAbfsRestOperationException(HTTP_PRECON_FAILED);
+
+    doThrow(conflictResponseEx) // Scn1: GFS fails with Http404
+        .doThrow(conflictResponseEx) // Scn2: GFS fails with Http500
+        .doThrow(
+            conflictResponseEx) // Scn3: create overwrite=true fails with Http412
+        .doThrow(
+            conflictResponseEx) // Scn4: create overwrite=true fails with Http500
+        .doThrow(
+            serverErrorResponseEx) // Scn5: create overwrite=false fails with Http500
+        .when(mockClient)
+        .createPathImpl(any(String.class), any(
+            AbfsUriQueryBuilder.class),
+            eq(false), any(String.class), any(String.class), eq(null));
+
+    doThrow(fileNotFoundResponseEx) // Scn1: GFS fails with Http404
+        .doThrow(serverErrorResponseEx) // Scn2: GFS fails with Http500
+        .doReturn(successOp) // Scn3: create overwrite=true fails with Http412
+        .doReturn(successOp) // Scn4: create overwrite=true fails with Http500
+        .when(mockClient)
+        .getPathStatus(any(String.class), eq(false));
+
+    doThrow(
+        preConditionResponseEx) // Scn3: create overwrite=true fails with Http412
+        .doThrow(
+            serverErrorResponseEx) // Scn4: create overwrite=true fails with Http500
+        .when(mockClient)
+        .createPathImpl(any(String.class), any(
+            AbfsUriQueryBuilder.class),
+            eq(true), any(String.class), any(String.class), eq(null));
+
+    when(mockClient.createPath(any(String.class), eq(true), eq(true),
+        any(String.class),
+        any(String.class), eq(false))).thenCallRealMethod();
+
+    // Scn1: GFS fails with Http404
+    // Sequence of events expected:
+    // 1. create overwrite=false - fail with conflict
+    // 2. GFS - fail with File Not found
+    // Create will fail with ConcurrentWriteOperationDetectedException
+    intercept(
+        ConcurrentWriteOperationDetectedException.class,
+        () ->
+            mockClient.createPath("someTestPath", true, true, "0644", "0022",
+                false));
+
+    // Scn2: GFS fails with Http500
+    // Sequence of events expected:
+    // 1. create overwrite=false - fail with conflict
+    // 2. GFS - fail with Server error
+    // Create will fail with 500
+    intercept(
+        AbfsRestOperationException.class,
+        () ->
+            mockClient.createPath("someTestPath", true, true, "0644", "0022",
+                false));
+
+    // Scn3: create overwrite=true fails with Http412
+    // Sequence of events expected:
+    // 1. create overwrite=false - fail with conflict
+    // 2. GFS - pass
+    // 3. create overwrite=true - fail with Pre-Condition
+    // Create will fail with ConcurrentWriteOperationDetectedException
+    intercept(
+        ConcurrentWriteOperationDetectedException.class,
+        () ->
+            mockClient.createPath("someTestPath", true, true, "0644", "0022",
+                false));
+
+    // Scn4: create overwrite=true fails with Http500
+    // Sequence of events expected:
+    // 1. create overwrite=false - fail with conflict
+    // 2. GFS - pass
+    // 3. create overwrite=true - fail with Server error
+    // Create will fail with 500
+    intercept(
+        AbfsRestOperationException.class,
+        () ->
+            mockClient.createPath("someTestPath", true, true, "0644", "0022",
+                false));
+
+    // Scn5: create overwrite=false fails with Http500
+    // Sequence of events expected:
+    // 1. create overwrite=false - fail with server error
+    // Create will fail with 500
+    intercept(
+        AbfsRestOperationException.class,
+        () ->
+            mockClient.createPath("someTestPath", true, true, "0644", "0022",
+                false));
+  }
+
+  private AbfsRestOperationException getMockAbfsRestOperationException(int status) {
+    return new AbfsRestOperationException(status, "", "", new Exception());
+  }
 }
diff --git a/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/services/TestAbfsClient.java b/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/services/TestAbfsClient.java
@@ -309,10 +309,19 @@ public static AbfsClient getMockAbfsClient(AbfsClient baseAbfsClientInstance,
     when(client.getSharedKeyCredentials()).thenCallRealMethod();
     when(client.createDefaultHeaders()).thenCallRealMethod();
 
+    // override baseurl
+    Field abfsConfigurationField = AbfsClient.class.getDeclaredField("abfsConfiguration");
+    abfsConfigurationField.setAccessible(true);
+    Field modifiersField = Field.class.getDeclaredField("modifiers");
+    modifiersField.setAccessible(true);
+    modifiersField.setInt(abfsConfigurationField,
+        abfsConfigurationField.getModifiers()
+            & ~java.lang.reflect.Modifier.FINAL);
+    abfsConfigurationField.set(client, abfsConfig);
+
     // override baseurl
     Field baseUrlField = AbfsClient.class.getDeclaredField("baseUrl");
     baseUrlField.setAccessible(true);
-    Field modifiersField = Field.class.getDeclaredField("modifiers");
     modifiersField.setAccessible(true);
     modifiersField.setInt(baseUrlField, baseUrlField.getModifiers() & ~java.lang.reflect.Modifier.FINAL);
     baseUrlField.set(client, baseAbfsClientInstance.getBaseUrl());
