Review-comments for pullRequest 5299

hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/constants/AbfsHttpConstants.java

@@ -112,12 +112,14 @@ public final class AbfsHttpConstants {
   public static final char CHAR_STAR = '*';
   public static final char CHAR_PLUS = '+';
   /**
-   * Value that differentiates categories of the http_status.<br>
-   * 100 - 199 : Informational responses<br>
-   * 200 - 299 : Successful responses<br>
-   * 300 - 399 : Redirection messages<br>
-   * 400 - 499 : Client error responses<br>
-   * 500 - 599 : Server error responses<br>
+   * Value that differentiates categories of the http_status.
+   * <pre>
+   * 100 - 199 : Informational responses
+   * 200 - 299 : Successful responses
+   * 300 - 399 : Redirection messages
+   * 400 - 499 : Client error responses
+   * 500 - 599 : Server error responses
+   * </pre>
    * */
   public static final Integer HTTP_STATUS_CATEGORY_QUOTIENT = 100;
 

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

@@ -66,6 +66,10 @@ public String getErrorCode() {
     return this.errorCode;
   }
 
+  public String getErrorMessage() {
+    return this.errorMessage;
+  }
+
   public static List<AzureServiceErrorCode> getAzureServiceCode(int httpStatusCode) {
     List<AzureServiceErrorCode> errorCodes = new ArrayList<>();
     if (httpStatusCode == UNKNOWN.httpStatusCode) {

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

@@ -74,7 +74,11 @@ public class AbfsRestOperation {
   private AbfsHttpOperation result;
   private AbfsCounters abfsCounters;
 
-  private String failureReason = null;
+  /**
+   * This variable contains the reason of last API call within the same
+   * AbfsRestOperation object.
+   * */
+  private String failureReason;
 
   /**
    * Checks if there is non-null HTTP response.
@@ -242,7 +246,7 @@ private void completeExecute(TracingContext tracingContext)
 
   @VisibleForTesting
   String getClientLatency() {
-    return this.client.getAbfsPerfTracker().getClientLatency();
+    return client.getAbfsPerfTracker().getClientLatency();
   }
 
   /**
@@ -256,7 +260,7 @@ private boolean executeHttpOperation(final int retryCount,
 
     try {
       // initialize the HTTP request and open the connection
-      httpOperation = getHttpOperation();
+      httpOperation = createHttpOperation();
       incrementCounter(AbfsStatistic.CONNECTIONS_MADE, 1);
       tracingContext.constructHeader(httpOperation, failureReason);
 
@@ -347,8 +351,12 @@ private boolean executeHttpOperation(final int retryCount,
     return true;
   }
 
+  /**
+   * Creates new object of {@link AbfsHttpOperation} with the url, method, and
+   * requestHeaders fields of the AbfsRestOperation object.
+   * */
   @VisibleForTesting
-  AbfsHttpOperation getHttpOperation() throws IOException {
+  AbfsHttpOperation createHttpOperation() throws IOException {
     return new AbfsHttpOperation(url, method, requestHeaders);
   }
 

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

@@ -18,159 +18,83 @@
 
 package org.apache.hadoop.fs.azurebfs.services;
 
-import java.io.IOException;
-import java.net.SocketException;
-import java.net.UnknownHostException;
-import java.util.ArrayList;
+import java.util.LinkedList;
 import java.util.List;
 
-import static java.net.HttpURLConnection.HTTP_UNAVAILABLE;
-import static org.apache.hadoop.fs.azurebfs.constants.AbfsHttpConstants.HTTP_STATUS_CATEGORY_QUOTIENT;
+import org.apache.hadoop.fs.azurebfs.services.retryReasonCategories.ClientErrorRetryReason;
+import org.apache.hadoop.fs.azurebfs.services.retryReasonCategories.ConnectionResetRetryReason;
+import org.apache.hadoop.fs.azurebfs.services.retryReasonCategories.ConnectionTimeoutRetryReason;
+import org.apache.hadoop.fs.azurebfs.services.retryReasonCategories.ReadTimeoutRetryReason;
+import org.apache.hadoop.fs.azurebfs.services.retryReasonCategories.RetryReasonCategory;
+import org.apache.hadoop.fs.azurebfs.services.retryReasonCategories.ServerErrorRetryReason;
+import org.apache.hadoop.fs.azurebfs.services.retryReasonCategories.UnknownHostRetryReason;
+import org.apache.hadoop.fs.azurebfs.services.retryReasonCategories.UnknownIOExceptionRetryReason;
+import org.apache.hadoop.fs.azurebfs.services.retryReasonCategories.UnknownSocketExceptionRetryReason;
+
 
 /**
- * In case of retry, this enum would give the information on the reason for
- * previous API call.
+ * This utility class exposes methods to convert a server response-error to a
+ * category of error.
  * */
-public enum RetryReason {
-  CONNECTION_TIMEOUT(2,
-      ((exceptionCaptured, statusCode, serverErrorMessage) -> {
-        if (exceptionCaptured != null && "connect timed out".equalsIgnoreCase(
-            exceptionCaptured.getMessage())) {
-          return "CT";
-        }
-        return null;
-      })),
-  READ_TIMEOUT(2, ((exceptionCaptured, statusCode, serverErrorMessage) -> {
-    if (exceptionCaptured != null && "Read timed out".equalsIgnoreCase(
-        exceptionCaptured.getMessage())) {
-      return "RT";
-    }
-    return null;
-  })),
-  UNKNOWN_HOST(2, ((exceptionCaptured, statusCode, serverErrorMessage) -> {
-    if (exceptionCaptured instanceof UnknownHostException) {
-      return "UH";
-    }
-    return null;
-  })),
-  CONNECTION_RESET(2, ((exceptionCaptured, statusCode, serverErrorMessage) -> {
-    if (exceptionCaptured != null && exceptionCaptured.getMessage() != null
-        && exceptionCaptured.getMessage().contains("Connection reset")) {
-      return "CR";
-    }
-    return null;
-  })),
-  STATUS_5XX(0, ((exceptionCaptured, statusCode, serverErrorMessage) -> {
-    if (statusCode == null || statusCode / HTTP_STATUS_CATEGORY_QUOTIENT != 5) {
-      return null;
-    }
-    if (statusCode == HTTP_UNAVAILABLE) {
-      serverErrorMessage = serverErrorMessage.split(System.lineSeparator(),
-          2)[0];
-      if ("Ingress is over the account limit.".equalsIgnoreCase(
-          serverErrorMessage)) {
-        return "ING";
-      }
-      if ("Egress is over the account limit.".equalsIgnoreCase(
-          serverErrorMessage)) {
-        return "EGR";
-      }
-      if ("Operations per second is over the account limit.".equalsIgnoreCase(
-          serverErrorMessage)) {
-        return "OPR";
-      }
-      return HTTP_UNAVAILABLE + "";
-    }
-    return statusCode + "";
-  })),
-  STATUS_4XX(0, ((exceptionCaptured, statusCode, serverErrorMessage) -> {
-    if (statusCode == null || statusCode / HTTP_STATUS_CATEGORY_QUOTIENT != 4) {
-      return null;
-    }
-    return statusCode + "";
-  })),
-  UNKNOWN_SOCKET_EXCEPTION(1,
-      ((exceptionCaptured, statusCode, serverErrorMessage) -> {
-        if (exceptionCaptured instanceof SocketException) {
-          return "SE";
-        }
-        return null;
-      })),
-  UNKNOWN_IO_EXCEPTION(0,
-      ((exceptionCaptured, statusCode, serverErrorMessage) -> {
-        if (exceptionCaptured instanceof IOException) {
-          return "IOE";
-        }
-        return null;
-      }));
-
-  private RetryReasonAbbreviationCreator retryReasonAbbreviationCreator = null;
-
-  private int rank = 0;
+class RetryReason {
 
   /**
-   * Constructor to have rank and the implementation of {@link RetryReasonAbbreviationCreator}.
-   * @param rank rank of a given enum. For example SocketTimeoutException is
-   * subclass of IOException. Rank of SocketTimeoutException enum has to be
-   * more than that of IOException enum.
-   * @param abbreviationCreator The implementation of {@link RetryReasonAbbreviationCreator}
-   * which would give the information if a given enum can be mapped to an error or not.
+   * Linked-list of the implementations of RetryReasonCategory. The objects in the
+   * list are arranged by the rank of their significance.
+   * <ul>
+   *   <li>ServerError (statusCode==5XX), ClientError (statusCode==4XX) are
+   *   independent of other retryReason categories.</li>
+   *   <li>Since {@link java.net.SocketException} is subclass of
+   *   {@link java.io.IOException},
+   *   hence, {@link UnknownIOExceptionRetryReason} is placed before
+   *   {@link UnknownSocketExceptionRetryReason}</li>
+   *   <li>Since, connectionTimeout, readTimeout, and connectionReset are
+   *   {@link java.net.SocketTimeoutException} exceptions with different messages,
+   *   hence, {@link ConnectionTimeoutRetryReason}, {@link ReadTimeoutRetryReason},
+   *   {@link ConnectionResetRetryReason} are above {@link UnknownIOExceptionRetryReason}.
+   *   There is no order between the three reasons as they are differentiated
+   *   by exception-message.</li>
+   *   <li>Since, {@link java.net.UnknownHostException} is subclass of
+   *   {@link java.io.IOException}, {@link UnknownHostRetryReason} is placed
+   *   over {@link UnknownIOExceptionRetryReason}</li>
+   * </ul>
    * */
-  RetryReason(int rank,
-      RetryReasonAbbreviationCreator abbreviationCreator) {
-    this.rank = rank;
-    this.retryReasonAbbreviationCreator = abbreviationCreator;
-  }
+  private static List<RetryReasonCategory> rankedReasonCategories
+      = new LinkedList<RetryReasonCategory>() {{
+    add(new ServerErrorRetryReason());
+    add(new ClientErrorRetryReason());
+    add(new UnknownIOExceptionRetryReason());
+    add(new UnknownSocketExceptionRetryReason());
+    add(new ConnectionTimeoutRetryReason());
+    add(new ReadTimeoutRetryReason());
+    add(new UnknownHostRetryReason());
+    add(new ConnectionResetRetryReason());
+  }};
 
-  private static List<RetryReason> retryReasonSortedList;
+  private RetryReason() {
 
-  /**
-   * Synchronized method to assign sorted list in {@link RetryReason#retryReasonSortedList}.
-   * Method would check if list is assigned or not. If yes, method would return. This is required
-   * because multiple threads could be waiting to get into this method, and once a thread is done
-   * with this method, other thread would get into this method. Since the list would be assigned by
-   * first thread, the second thread need not run the whole mechanism of sorting.
-   * The enums are sorted on the ascending order of their rank.
-   * */
-  private static synchronized void sortRetryReason() {
-    if (retryReasonSortedList != null) {
-      return;
-    }
-    List<RetryReason> list = new ArrayList<>();
-    for (RetryReason reason : values()) {
-      list.add(reason);
-    }
-    list.sort((c1, c2) -> {
-      return c1.rank - c2.rank;
-    });
-    retryReasonSortedList = list;
   }
 
   /**
    * Method to get correct abbreviation for a given set of exception, statusCode,
    * storageStatusCode.
-   * Method would iterate through the {@link RetryReason#retryReasonSortedList},
-   * and would return the abbreviation returned by highest enum to be applicable on the group.
-   * For example, if SocketTimeoutException(rank 2) and IOException(rank 0) can be
-   * applied on the group, the abbreviation of SocketTimeoutException has to be returned.
    *
    * @param ex exception caught during server communication.
    * @param statusCode statusCode in the server response.
    * @param storageErrorMessage storageErrorMessage in the server response.
+   *
+   * @return abbreviation for the the given set of exception, statusCode, storageStatusCode.
    * */
   static String getAbbreviation(Exception ex,
       Integer statusCode,
       String storageErrorMessage) {
     String result = null;
-    if (retryReasonSortedList == null) {
-      sortRetryReason();
-    }
-    for (RetryReason retryReason : retryReasonSortedList) {
-      String enumCapturedAndAbbreviate
-          = retryReason.retryReasonAbbreviationCreator.capturableAndGetAbbreviation(
-          ex, statusCode, storageErrorMessage);
-      if (enumCapturedAndAbbreviate != null) {
-        result = enumCapturedAndAbbreviate;
+    for (RetryReasonCategory retryReasonCategory : rankedReasonCategories) {
+      final String abbreviation
+          = retryReasonCategory.captureAndGetAbbreviation(ex,
+          statusCode, storageErrorMessage);
+      if (abbreviation != null) {
+        result = abbreviation;
       }
     }
     return result;

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

@@ -0,0 +1,35 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.fs.azurebfs.services;
+
+public class RetryReasonConstants {
+  public static final String CONNECTION_TIMEOUT_JDK_MESSAGE = "connect timed out";
+  public static final String READ_TIMEOUT_JDK_MESSAGE = "Read timed out";
+  public static final String CONNECTION_RESET_MESSAGE = "Connection reset";
+  public static final String OPERATION_BREACH_MESSAGE = "Operations per second is over the account limit.";
+  public static final String CONNECTION_RESET_ABBREVIATION = "CR";
+  public static final String CONNECTION_TIMEOUT_ABBREVIATION = "CT";
+  public static final String READ_TIMEOUT_ABBREVIATION = "RT";
+  public static final String INGRESS_LIMIT_BREACH_ABBREVIATION = "ING";
+  public static final String EGRESS_LIMIT_BREACH_ABBREVIATION = "EGR";
+  public static final String OPERATION_LIMIT_BREACH_ABBREVIATION = "OPR";
+  public static final String UNKNOWN_HOST_EXCEPTION_ABBREVIATION = "UH";
+  public static final String IO_EXCEPTION_ABBREVIATION = "IOE";
+  public static final String SOCKET_EXCEPTION_ABBREVIATION = "SE";
+}