[GitHub] [hive] t3rmin4t0r commented on a change in pull request #1983: HIVE-24786: JDBC HttpClient should retry for idempotent and unsent http methods

[GitBox <gi...@apache.org>]

t3rmin4t0r commented on a change in pull request #1983:
URL: https://github.com/apache/hive/pull/1983#discussion_r577852301



##########
File path: jdbc/src/java/org/apache/hive/jdbc/HiveConnection.java
##########
@@ -581,21 +596,99 @@ public long getRetryInterval() {
     } else {
       httpClientBuilder = HttpClientBuilder.create();
     }
-    // In case the server's idletimeout is set to a lower value, it might close it's side of
-    // connection. However we retry one more time on NoHttpResponseException
+
+    // Beeline <------> LB <------> Reverse Proxy <-----> Hiveserver2
+    // In case of deployments like above, the LoadBalancer (LB) can be configured with Idle Timeout after which the LB
+    // will send TCP RST to Client (Beeline) and Backend (Reverse Proxy). If user is connected to beeline, idle for
+    // sometime and resubmits a query after the idle timeout there is a broken pipe between beeline and LB. When Beeline
+    // tries to submit the query one of two things happen, it either hangs or times out (if socketTimeout is defined in
+    // the jdbc param). The hang is because of the default infinite socket timeout for which there is no auto-recovery
+    // (user have to manually interrupt the query). If the socketTimeout jdbc param was specified, beeline will receive
+    // SocketTimeoutException (Read Timeout) or NoHttpResponseException both of which can be retried if maxRetries is
+    // also specified by the user (jdbc param).
+    // The following retry handler handles the above cases in addition to retries for idempotent and unsent requests.
     httpClientBuilder.setRetryHandler(new HttpRequestRetryHandler() {
+      // This handler is mostly a copy of DefaultHttpRequestRetryHandler except it also retries some exceptions
+      // which could be thrown in certain cases where idle timeout from intermediate proxy triggers a connection reset.
+      private final List<Class<? extends IOException>> nonRetriableClasses = Arrays.asList(
+              InterruptedIOException.class,
+              UnknownHostException.class,
+              ConnectException.class,
+              SSLException.class);
+      // socket exceptions could happen because of timeout, broken pipe or server not responding in which case it is
+      // better to reopen the connection and retry if user specified maxRetries
+      private final List<Class<? extends IOException>> retriableClasses = Arrays.asList(
+              SocketTimeoutException.class,
+              SocketException.class,
+              NoHttpResponseException.class
+      );
+
       @Override
       public boolean retryRequest(IOException exception, int executionCount, HttpContext context) {
-        if (executionCount > 1) {
-          LOG.info("Retry attempts to connect to server exceeded.");
+        Args.notNull(exception, "Exception parameter");
+        Args.notNull(context, "HTTP context");
+        if (executionCount > maxRetries) {
+          // Do not retry if over max retry count
+          LOG.error("Max retries (" + maxRetries + ") exhausted.", exception);
+          return false;
+        }
+        if (this.retriableClasses.contains(exception.getClass())) {
+          LOG.info("Retrying " + exception.getClass() + " as it is in retriable classes list.");
+          return true;
+        }
+        if (this.nonRetriableClasses.contains(exception.getClass())) {
+          LOG.info("Not retrying as the class (" + exception.getClass() + ") is non-retriable class.");
+          return false;
+        } else {
+          for (final Class<? extends IOException> rejectException : this.nonRetriableClasses) {
+            if (rejectException.isInstance(exception)) {
+              LOG.info("Not retrying as the class (" + exception.getClass() + ") is an instance of is non-retriable class.");;
+              return false;
+            }
+          }
+        }
+        final HttpClientContext clientContext = HttpClientContext.adapt(context);
+        final HttpRequest request = clientContext.getRequest();
+
+        if(requestIsAborted(request)){
+          LOG.info("Not retrying as request is aborted.");
           return false;
         }
-        if (exception instanceof org.apache.http.NoHttpResponseException) {
-          LOG.info("Could not connect to the server. Retrying one more time.");
+
+        if (handleAsIdempotent(request)) {
+          LOG.info("Retrying idempotent request. Attempt " + executionCount + " of " + maxRetries);
+          // Retry if the request is considered idempotent
+          return true;
+        }
+
+        if (!clientContext.isRequestSent()) {
+          LOG.info("Retrying unsent request. Attempt " + executionCount + " of " + maxRetries);
+          // Retry if the request has not been sent fully or
+          // if it's OK to retry methods that have been sent
           return true;
         }
+
+        LOG.info("Not retrying as the request is not idempotent or is already sent.");
+        // otherwise do not retry
         return false;
       }
+
+      // requests that handles "Expect continue" handshakes. If server received the header and is waiting for body
+      // then those requests can be retried. Most basic http method methods except DELETE are idempotent as long as they

Review comment:
       Specifically, a "POST" body we haven't sent yet hasn't left any unclean state on the server side (in case of a network error on response). 




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org