[ 
https://issues.apache.org/jira/browse/HADOOP-18915?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17790691#comment-17790691
 ] 

ASF GitHub Bot commented on HADOOP-18915:
-----------------------------------------

steveloughran commented on code in PR #6180:
URL: https://github.com/apache/hadoop/pull/6180#discussion_r1408127824


##########
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/impl/AWSClientConfig.java:
##########
@@ -371,24 +407,219 @@ private static void initSigner(Configuration conf,
   }
 
   /**
-   * Configures request timeout.
+   * Configures request timeout in the client configuration.
+   * This is independent of the timeouts set in the sync and async HTTP 
clients;
+   * the same method
    *
    * @param conf Hadoop configuration
    * @param clientConfig AWS SDK configuration to update
    */
   private static void initRequestTimeout(Configuration conf,
       ClientOverrideConfiguration.Builder clientConfig) {
-    long requestTimeoutMillis = conf.getTimeDuration(REQUEST_TIMEOUT,
-        DEFAULT_REQUEST_TIMEOUT, TimeUnit.SECONDS, TimeUnit.MILLISECONDS);
+    // Get the connection settings
+    final ConnectionSettings conn = createApiConnectionSettings(conf);
+    final Duration callTimeout = conn.getApiCallTimeout();
 
-    if (requestTimeoutMillis > Integer.MAX_VALUE) {
-      LOG.debug("Request timeout is too high({} ms). Setting to {} ms instead",
-          requestTimeoutMillis, Integer.MAX_VALUE);
-      requestTimeoutMillis = Integer.MAX_VALUE;
+    if (callTimeout.toMillis() > 0) {
+      clientConfig.apiCallAttemptTimeout(callTimeout);
+      clientConfig.apiCallTimeout(callTimeout);
     }
+  }
 
-    if(requestTimeoutMillis > 0) {
-      
clientConfig.apiCallAttemptTimeout(Duration.ofMillis(requestTimeoutMillis));
+  /**
+   * Reset the minimum operation duration to the default.
+   * Logs at INFO.
+   * <p>
+   * This MUST be called in test teardown in any test suite which
+   * called {@link #setMinimumOperationDuration(Duration)}.
+   */
+  public static void resetMinimumOperationDuration() {
+    setMinimumOperationDuration(MINIMUM_NETWORK_OPERATION_DURATION);
+  }
+
+  /**
+   * Set the minimum operation duration.
+   * This is for testing and will log at info; does require a non-negative 
duration.
+   * <p>
+   * Test suites must call {@link #resetMinimumOperationDuration()} in their 
teardown
+   * to avoid breaking subsequent tests in the same process.
+   * @param duration non-negative duration
+   * @throws IllegalArgumentException if the duration is negative.
+   */
+  @VisibleForTesting
+  public static void setMinimumOperationDuration(Duration duration) {
+    LOG.info("Setting minimum operation duration to {}ms", 
duration.toMillis());
+    checkArgument(duration.compareTo(Duration.ZERO) >= 0,
+        "Duration must be positive: %sms", duration.toMillis());
+    minimumOperationDuration = duration;
+  }
+
+  /**
+   * Get the current minimum operation duration.
+   * @return current duration.
+   */
+  public static Duration getMinimumOperationDuration() {
+    return minimumOperationDuration;
+  }
+
+  /**
+   * All the connection settings, wrapped as a class for use by
+   * both the sync and async clients, and connection client builder.
+   */
+  static class ConnectionSettings {
+    private final int maxConnections;
+    private final boolean keepAlive;
+    private final Duration acquisitionTimeout;
+    private final Duration apiCallTimeout;
+    private final Duration connectionTTL;
+    private final Duration establishTimeout;
+    private final Duration maxIdleTime;
+    private final Duration socketTimeout;
+
+    ConnectionSettings(
+        final int maxConnections,
+        final boolean keepAlive,
+        final Duration apiCallTimeout,
+        final Duration acquisitionTimeout,
+        final Duration connectionTTL,
+        final Duration establishTimeout,
+        final Duration maxIdleTime,
+        final Duration socketTimeout) {
+      this.maxConnections = maxConnections;
+      this.keepAlive = keepAlive;
+      this.acquisitionTimeout = acquisitionTimeout;
+      this.apiCallTimeout = apiCallTimeout;
+      this.connectionTTL = connectionTTL;
+      this.establishTimeout = establishTimeout;
+      this.maxIdleTime = maxIdleTime;
+      this.socketTimeout = socketTimeout;
+    }
+
+    int getMaxConnections() {
+      return maxConnections;
+    }
+
+    boolean isKeepAlive() {
+      return keepAlive;
+    }
+
+    Duration getAcquisitionTimeout() {
+      return acquisitionTimeout;
+    }
+
+    Duration getApiCallTimeout() {
+      return apiCallTimeout;
+    }
+
+    Duration getConnectionTTL() {
+      return connectionTTL;
+    }
+
+    Duration getEstablishTimeout() {
+      return establishTimeout;
+    }
+
+    Duration getMaxIdleTime() {
+      return maxIdleTime;
+    }
+
+    Duration getSocketTimeout() {
+      return socketTimeout;
+    }
+
+    @Override
+    public String toString() {
+      return "ConnectionSettings{" +
+          "maxConnections=" + maxConnections +
+          ", keepAlive=" + keepAlive +
+          ", acquisitionTimeout=" + acquisitionTimeout +
+          ", apiCallTimeout=" + apiCallTimeout +
+          ", connectionTTL=" + connectionTTL +
+          ", establishTimeout=" + establishTimeout +
+          ", maxIdleTime=" + maxIdleTime +
+          ", socketTimeout=" + socketTimeout +
+          '}';
     }
   }
+
+
+  /**
+   * Build a connection settings object with only the settings used
+   * for the ClientConfig only.
+   * All other fields are null and MUST NOT be used.
+   * @param conf configuration to evaluate
+   * @return connection settings.
+   */
+  static ConnectionSettings createApiConnectionSettings(Configuration conf) {

Review Comment:
   I've kept in in one place in case they ever need adding. I can add a new 
ClientConnectionSettings struct there (which would be so much easier in java17)





> Extend S3A http client connection timeouts
> ------------------------------------------
>
>                 Key: HADOOP-18915
>                 URL: https://issues.apache.org/jira/browse/HADOOP-18915
>             Project: Hadoop Common
>          Issue Type: Sub-task
>          Components: fs/s3
>    Affects Versions: 3.4.0
>            Reporter: Ahmar Suhail
>            Assignee: Steve Loughran
>            Priority: Major
>              Labels: pull-request-available
>
> * Add ability to configure *all* timeouts, especially acquisition time
> * recognise ApiCallTimeout and map tp a retryable exception
> * use getDuration so suffixes can be used -so remove all ambiguity about time 
> unit
> * use units in core-default.xml so warnings aren't printed



--
This message was sent by Atlassian Jira
(v8.20.10#820010)

---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to