Test Failures Fixes

Author: Anuj Modi

hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsRestOperation.java

@@ -39,7 +39,6 @@
 import org.apache.hadoop.fs.statistics.impl.IOStatisticsBinding;
 
 import static org.apache.hadoop.fs.azurebfs.constants.AbfsHttpConstants.HTTP_CONTINUE;
-import static org.apache.hadoop.fs.azurebfs.services.RetryReasonConstants.CONNECTION_TIMEOUT_ABBREVIATION;
 import static org.apache.hadoop.fs.azurebfs.services.RetryReasonConstants.EGRESS_LIMIT_BREACH_ABBREVIATION;
 import static org.apache.hadoop.fs.azurebfs.services.RetryReasonConstants.INGRESS_LIMIT_BREACH_ABBREVIATION;
 import static org.apache.hadoop.fs.azurebfs.services.RetryReasonConstants.OPERATION_LIMIT_BREACH_ABBREVIATION;
@@ -325,14 +324,18 @@ private boolean executeHttpOperation(final int retryCount,
         incrementCounter(AbfsStatistic.SERVER_UNAVAILABLE, 1);
       }
 
-      // If no exception occurred here it means http operation was successfully complete and
+      // If no exception occurred till here it means http operation was successfully complete and
       // a response from server has been received which might be failure or success.
+      // If any kind of exception has occurred it will be caught below.
+      // If request failed determine failure reason and retry policy here.
+      // else simply return with success after saving the result.
       LOG.debug("HttpRequest: {}: {}", operationType, httpOperation);
 
-      // If request failed at server and should be retried, we will determine failure reason and retry policy here
+      int status = httpOperation.getStatusCode();
+      failureReason = RetryReason.getAbbreviation(null, status, httpOperation.getStorageErrorMessage());
+      retryPolicy = client.getRetryPolicy(failureReason);
+
       if (retryPolicy.shouldRetry(retryCount, httpOperation.getStatusCode())) {
-        int status = httpOperation.getStatusCode();
-        failureReason = RetryReason.getAbbreviation(null, status, httpOperation.getStorageErrorMessage());
         return false;
       }
 
@@ -370,7 +373,7 @@ private boolean executeHttpOperation(final int retryCount,
         1. Status code in 2xx range: Successful Operations should contribute
         2. Status code in 3xx range: Redirection Operations should not contribute
         3. Status code in 4xx range: User Errors should not contribute
-        4. Status code in 503 range: Server Error should contribute as following:
+        4. Status code is 503: Throttling Error should contribute as following:
           a. 503, Ingress Over Account Limit: Should Contribute
           b. 503, Egress Over Account Limit: Should Contribute
           c. 503, TPS Over Account Limit: Should Contribute

hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/services/ITestAbfsRestOperation.java

@@ -61,6 +61,7 @@
 import static org.apache.hadoop.fs.azurebfs.constants.HttpQueryParams.QUERY_PARAM_POSITION;
 import static org.apache.hadoop.fs.azurebfs.constants.TestConfigurationKeys.FS_AZURE_ABFS_ACCOUNT_NAME;
 import static org.apache.hadoop.fs.azurebfs.constants.TestConfigurationKeys.TEST_CONFIGURATION_FILE_NAME;
+import static org.apache.hadoop.fs.azurebfs.contracts.services.AzureServiceErrorCode.EGRESS_OVER_ACCOUNT_LIMIT;
 import static org.apache.hadoop.test.LambdaTestUtils.intercept;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.never;
@@ -233,6 +234,11 @@ private AbfsRestOperation getRestOperation() throws Exception {
       // mocked the response code and the response message to check different
       // behaviour based on response code.
       Mockito.doReturn(responseCode).when(abfsHttpOperation).getConnResponseCode();
+      if (responseCode == HTTP_UNAVAILABLE) {
+        Mockito.doReturn(EGRESS_OVER_ACCOUNT_LIMIT.getErrorMessage())
+            .when(abfsHttpOperation)
+            .getStorageErrorMessage();
+      }
       Mockito.doReturn(responseMessage)
           .when(abfsHttpOperation)
           .getConnResponseMessage();

hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/services/TestAbfsRestOperationMockFailures.java

@@ -192,7 +192,6 @@ public void testClientRequestIdFor503OtherRetry() throws Exception {
    * 2. Tracing header construction takes place with proper arguments based on the failure reason and retry policy used
    * @throws Exception
    */
-
   @Test
   public void testRetryPolicyWithDifferentFailureReasons() throws Exception {
 
@@ -226,27 +225,26 @@ public void testRetryPolicyWithDifferentFailureReasons() throws Exception {
     Mockito.doReturn("").when(httpOperation).getStorageErrorMessage();
     Mockito.doReturn("").when(httpOperation).getStorageErrorCode();
     Mockito.doReturn("HEAD").when(httpOperation).getMethod();
+    Mockito.doReturn(EGRESS_OVER_ACCOUNT_LIMIT.getErrorMessage()).when(httpOperation).getStorageErrorMessage();
     Mockito.doReturn(tracingContext).when(abfsRestOperation).createNewTracingContext(any());
 
     try {
       // Operation will fail with CT first and then 503 thereafter.
       abfsRestOperation.execute(tracingContext);
     } catch(AbfsRestOperationException ex) {
       Assertions.assertThat(ex.getStatusCode())
-          .describedAs("Status Code must be HTTP_UNAVAILABLE(409)")
+          .describedAs("Status Code must be HTTP_UNAVAILABLE(503)")
           .isEqualTo(HTTP_UNAVAILABLE);
     }
 
     // Assert that httpOperation.processResponse was called 3 times.
     // One for retry count 0
     // One for retry count 1 after failing with CT
-    // One for retry count 2 after failing with 50
+    // One for retry count 2 after failing with 503
     Mockito.verify(httpOperation, times(3)).processResponse(
         nullable(byte[].class), nullable(int.class), nullable(int.class));
 
-    // Assert that Static Retry Policy was used after CT failure.
-    // Iteration 1 failed with CT and shouldRetry was called with retry count 0
-    // Before iteration 2 sleep will be computed using static retry policy and retry count 1
+    // Primary Request Failed with CT. Static Retry Policy should be used.
     Mockito.verify(abfsClient, Mockito.times(1))
         .getRetryPolicy(CONNECTION_TIMEOUT_ABBREVIATION);
     Mockito.verify(staticRetryPolicy, Mockito.times(1))
@@ -261,17 +259,17 @@ public void testRetryPolicyWithDifferentFailureReasons() throws Exception {
     // Before iteration 3 sleep will be computed using exponential retry policy and retry count 2
     // Should retry with retry count 2 will return false and no further requests will be made.
     Mockito.verify(abfsClient, Mockito.times(2))
-        .getRetryPolicy("503");
+        .getRetryPolicy(EGRESS_LIMIT_BREACH_ABBREVIATION);
     Mockito.verify(exponentialRetryPolicy, Mockito.times(1))
         .shouldRetry(1, HTTP_UNAVAILABLE);
     Mockito.verify(exponentialRetryPolicy, Mockito.times(1))
         .shouldRetry(2, HTTP_UNAVAILABLE);
     Mockito.verify(exponentialRetryPolicy, Mockito.times(1))
         .getRetryInterval(2);
     Mockito.verify(tracingContext, Mockito.times(1))
-        .constructHeader(httpOperation, "503", EXPONENTIAL_RETRY_POLICY_ABBREVIATION);
+        .constructHeader(httpOperation, EGRESS_LIMIT_BREACH_ABBREVIATION, EXPONENTIAL_RETRY_POLICY_ABBREVIATION);
 
-    // Assert that intercept.updateMetrics was called only once during second Iteration
+    // Assert that intercept.updateMetrics was called 2 times. Both the retried request fails with EGR.
     Mockito.verify(intercept, Mockito.times(2))
         .updateMetrics(nullable(AbfsRestOperationType.class), nullable(AbfsHttpOperation.class));
   }