Phase 2 , getting Apache 5 compilation and Junit ready along with clearing Checkstyles and spotbug issues (#6100)

* Phase 2 , getting Apache 5 compilation and Junit ready along with clearing Checkstyles and spotbug issues

* Handle comments from review

* Handle comments from Zoe

Author: joviegas

build-tools/src/main/resources/software/amazon/awssdk/spotbugs-suppressions.xml

@@ -285,6 +285,7 @@
             <Class name="software.amazon.awssdk.core.util.DefaultSdkAutoConstructMap"/>
             <Class name="software.amazon.awssdk.http.nio.netty.internal.http2.FlushOnReadHandler"/>
             <Class name="software.amazon.awssdk.http.apache.internal.conn.IdleConnectionReaper"/>
+            <Class name="software.amazon.awssdk.http.apache5.internal.conn.IdleConnectionReaper"/>
             <Class name="software.amazon.awssdk.eventnotifications.s3.internal.DefaultS3EventNotificationWriter"/>
             <Class name="software.amazon.awssdk.policybuilder.iam.internal.DefaultIamPolicyWriter"/>
         </Or>
@@ -337,6 +338,7 @@
             <Class name="~software\.amazon\.awssdk\.utils\.CompletableFutureUtils" />
             <Class name="~software\.amazon\.awssdk\.metrics\.publishers\.cloudwatch\.CloudWatchMetricPublisher" />
             <Class name="~software\.amazon\.awssdk\.http\.apache\.internal\.conn\.IdleConnectionReaper\$ReaperTask" />
+            <Class name="~software\.amazon\.awssdk\.http\.apache5\.internal\.conn\.IdleConnectionReaper\$ReaperTask" />
             <Class name="~software\.amazon\.awssdk\.core\.internal\.retry\.RateLimitingTokenBucket" />
             <Class name="~software\.amazon\.awssdk\.core\.internal\.waiters\.WaiterExecutor" />
             <Class name="~software\.amazon\.awssdk\.regions\.internal\.util\.EC2MetadataUtils" />

http-clients/apache5-client/pom.xml

@@ -43,12 +43,51 @@
         <artifactId>httpcore5</artifactId>
         <version>5.3.4</version>
     </dependency>
+    <dependency>
+        <groupId>software.amazon.awssdk</groupId>
+        <artifactId>utils</artifactId>
+        <version>${awsjavasdk.version}</version>
+    </dependency>
+    <dependency>
+        <groupId>software.amazon.awssdk</groupId>
+        <artifactId>metrics-spi</artifactId>
+        <version>${awsjavasdk.version}</version>
+    </dependency>
+    <dependency>
+        <groupId>software.amazon.awssdk</groupId>
+        <artifactId>annotations</artifactId>
+        <version>${awsjavasdk.version}</version>
+    </dependency>
+
     <dependency>
         <groupId>software.amazon.awssdk</groupId>
         <artifactId>http-client-tests</artifactId>
         <version>${awsjavasdk.version}</version>
         <scope>test</scope>
     </dependency>
+    <!-- Added for WIRE logging details while testing-->
+    <dependency>
+        <groupId>org.apache.logging.log4j</groupId>
+        <artifactId>log4j-api</artifactId>
+        <scope>test</scope>
+    </dependency>
+    <dependency>
+        <groupId>org.apache.logging.log4j</groupId>
+        <artifactId>log4j-core</artifactId>
+        <scope>test</scope>
+    </dependency>
+    <dependency>
+        <groupId>org.apache.logging.log4j</groupId>
+        <artifactId>log4j-slf4j-impl</artifactId>
+        <scope>test</scope>
+    </dependency>
+    <dependency>
+        <groupId>org.slf4j</groupId>
+        <artifactId>jcl-over-slf4j</artifactId>
+        <scope>test</scope>
+        <version>${slf4j.version}</version>
+    </dependency>
+
     <dependency>
         <groupId>org.junit.jupiter</groupId>
         <artifactId>junit-jupiter</artifactId>

http-clients/apache5-client/src/main/java/software/amazon/awssdk/http/apache5/Apache5HttpClient.java

@@ -22,7 +22,7 @@
 import java.io.IOException;
 import java.io.InputStream;
 import java.io.OutputStream;
-import java.util.Optional;
+import org.apache.hc.core5.http.ContentType;
 import org.apache.hc.core5.http.io.entity.BasicHttpEntity;
 import org.apache.hc.core5.http.io.entity.InputStreamEntity;
 import software.amazon.awssdk.annotations.SdkInternalApi;
@@ -47,10 +47,6 @@ public class RepeatableInputStreamRequestEntity extends BasicHttpEntity {
      */
     private boolean firstAttempt = true;
 
-    /**
-     * True if the "Transfer-Encoding:chunked" header is present
-     */
-    private boolean isChunked;
 
     /**
      * The underlying InputStreamEntity being delegated to
@@ -80,33 +76,32 @@ public class RepeatableInputStreamRequestEntity extends BasicHttpEntity {
      * @param request The details of the request being written out (content type,
      *                content length, and content).
      */
-    public RepeatableInputStreamRequestEntity(final HttpExecuteRequest request) {
-        isChunked = request.httpRequest().matchingHeaders(TRANSFER_ENCODING).contains(CHUNKED);
-        setChunked(isChunked);
-
-        /*
-         * If we don't specify a content length when we instantiate our
-         * InputStreamRequestEntity, then HttpClient will attempt to
-         * buffer the entire stream contents into memory to determine
-         * the content length.
-         */
-        long contentLength = request.httpRequest().firstMatchingHeader("Content-Length")
-                                    .map(this::parseContentLength)
-                                    .orElse(-1L);
-
-        content = getContent(request.contentStreamProvider());
-        // TODO v2 MetricInputStreamEntity
-        inputStreamRequestEntity = new InputStreamEntity(content, contentLength);
-        setContent(content);
-        setContentLength(contentLength);
-
-        request.httpRequest().firstMatchingHeader("Content-Type").ifPresent(contentType -> {
-            inputStreamRequestEntity.setContentType(contentType);
-            setContentType(contentType);
-        });
+    public RepeatableInputStreamRequestEntity(HttpExecuteRequest request) {
+        super(request.contentStreamProvider().map(ContentStreamProvider::newStream)
+                     .orElseGet(() -> new ByteArrayInputStream(new byte[0])),
+              getContentLengthFromRequest(request),
+              getContentTypeFromRequest(request),
+              null,
+              request.httpRequest().matchingHeaders(TRANSFER_ENCODING).contains(CHUNKED));
+        InputStream inputStream = getContent();
+        this.content = inputStream;
+        inputStreamRequestEntity = new InputStreamEntity(inputStream,
+                                                         getContentLengthFromRequest(request),
+                                                         getContentTypeFromRequest(request));
+    }
+
+
+    private static ContentType getContentTypeFromRequest(HttpExecuteRequest request) {
+        return request.httpRequest().firstMatchingHeader("Content-Type").map(ContentType::create).orElse(null);
     }
 
-    private long parseContentLength(String contentLength) {
+    private static Long getContentLengthFromRequest(HttpExecuteRequest request) {
+        return request.httpRequest().firstMatchingHeader("Content-Length")
+                      .map(RepeatableInputStreamRequestEntity::parseContentLength)
+                      .orElse(-1L);
+    }
+
+    private static long parseContentLength(String contentLength) {
         try {
             return Long.parseLong(contentLength);
         } catch (NumberFormatException nfe) {
@@ -115,25 +110,9 @@ private long parseContentLength(String contentLength) {
         }
     }
 
-    /**
-     * @return The request content input stream or an empty input stream if there is no content.
-     */
-    private InputStream getContent(Optional<ContentStreamProvider> contentStreamProvider) {
-        return contentStreamProvider.map(ContentStreamProvider::newStream).orElseGet(() -> new ByteArrayInputStream(new byte[0]));
-    }
-
-    @Override
-    public boolean isChunked() {
-        return isChunked;
-    }
 
-    /**
-     * Returns true if the underlying InputStream supports marking/reseting or
-     * if the underlying InputStreamRequestEntity is repeatable.
-     */
-    @Override
-    public boolean isRepeatable() {
-        return content.markSupported() || inputStreamRequestEntity.isRepeatable();
+    private boolean isRepeatableStream() {
+        return getContent().markSupported() || inputStreamRequestEntity.isRepeatable();
     }
 
     /**
@@ -149,7 +128,8 @@ public boolean isRepeatable() {
     @Override
     public void writeTo(OutputStream output) throws IOException {
         try {
-            if (!firstAttempt && isRepeatable()) {
+            if (!firstAttempt && isRepeatableStream()) {
+
                 content.reset();
             }
 

http-clients/apache5-client/src/main/java/software/amazon/awssdk/http/apache5/internal/RepeatableInputStreamRequestEntity.java

@@ -22,7 +22,6 @@
 import org.apache.hc.client5.http.impl.routing.DefaultRoutePlanner;
 import org.apache.hc.core5.http.HttpException;
 import org.apache.hc.core5.http.HttpHost;
-import org.apache.hc.core5.http.HttpRequest;
 import org.apache.hc.core5.http.protocol.HttpContext;
 import software.amazon.awssdk.annotations.SdkInternalApi;
 

http-clients/apache5-client/src/main/java/software/amazon/awssdk/http/apache5/internal/SdkProxyRoutePlanner.java

@@ -16,17 +16,20 @@
 package software.amazon.awssdk.http.apache5.internal.conn;
 
 import java.io.IOException;
-import java.util.concurrent.TimeUnit;
-import org.apache.hc.client5.http.ConnectionRequest;
 import org.apache.hc.client5.http.HttpRoute;
+import org.apache.hc.client5.http.io.ConnectionEndpoint;
 import org.apache.hc.client5.http.io.HttpClientConnectionManager;
-import org.apache.hc.core5.http.io.HttpClientConnection;
+import org.apache.hc.client5.http.io.LeaseRequest;
 import org.apache.hc.core5.http.protocol.HttpContext;
+import org.apache.hc.core5.io.CloseMode;
+import org.apache.hc.core5.util.TimeValue;
+import org.apache.hc.core5.util.Timeout;
 import software.amazon.awssdk.annotations.SdkInternalApi;
 
 @SdkInternalApi
 public final class ClientConnectionManagerFactory {
 
+
     private ClientConnectionManagerFactory() {
     }
 
@@ -44,7 +47,7 @@ public static HttpClientConnectionManager wrap(HttpClientConnectionManager orig)
     }
 
     /**
-     * Further wraps {@link ConnectionRequest} to capture performance metrics.
+     * Further wraps {@link LeaseRequest} to capture performance metrics.
      */
     private static class InstrumentedHttpClientConnectionManager extends DelegatingHttpClientConnectionManager {
 
@@ -53,10 +56,12 @@ private InstrumentedHttpClientConnectionManager(HttpClientConnectionManager dele
         }
 
         @Override
-        public ConnectionRequest requestConnection(HttpRoute route, Object state) {
-            ConnectionRequest connectionRequest = super.requestConnection(route, state);
+        public LeaseRequest lease(String id, HttpRoute route, Timeout requestTimeout, Object state) {
+            LeaseRequest connectionRequest =  super.lease(id, route, requestTimeout, state);
             return ClientConnectionRequestFactory.wrap(connectionRequest);
         }
+
+
     }
 
     /**
@@ -71,44 +76,37 @@ protected DelegatingHttpClientConnectionManager(HttpClientConnectionManager dele
         }
 
         @Override
-        public ConnectionRequest requestConnection(HttpRoute route, Object state) {
-            return delegate.requestConnection(route, state);
+        public LeaseRequest lease(String id, HttpRoute route, Timeout requestTimeout, Object state) {
+            return delegate.lease(id, route, requestTimeout, state);
         }
 
         @Override
-        public void releaseConnection(HttpClientConnection conn, Object newState, long validDuration, TimeUnit timeUnit) {
-            delegate.releaseConnection(conn, newState, validDuration, timeUnit);
-        }
+        public void release(ConnectionEndpoint endpoint, Object newState, TimeValue validDuration) {
+            delegate.release(endpoint, newState, validDuration);
 
-        @Override
-        public void connect(HttpClientConnection conn, HttpRoute route, int connectTimeout, HttpContext context)
-                throws IOException {
-            delegate.connect(conn, route, connectTimeout, context);
         }
 
         @Override
-        public void upgrade(HttpClientConnection conn, HttpRoute route, HttpContext context) throws IOException {
-            delegate.upgrade(conn, route, context);
-        }
+        public void connect(ConnectionEndpoint endpoint, TimeValue connectTimeout, HttpContext context) throws IOException {
+            delegate.connect(endpoint, connectTimeout, context);
 
-        @Override
-        public void routeComplete(HttpClientConnection conn, HttpRoute route, HttpContext context) throws IOException {
-            delegate.routeComplete(conn, route, context);
         }
 
         @Override
-        public void closeIdleConnections(long idletime, TimeUnit timeUnit) {
-            delegate.closeIdleConnections(idletime, timeUnit);
+        public void upgrade(ConnectionEndpoint endpoint, HttpContext context) throws IOException {
+            delegate.upgrade(endpoint, context);
         }
 
         @Override
-        public void closeExpiredConnections() {
-            delegate.closeExpiredConnections();
+        public void close(CloseMode closeMode) {
+            delegate.close(closeMode);
+
         }
 
         @Override
-        public void shutdown() {
-            delegate.shutdown();
+        public void close() throws IOException {
+            delegate.close();
+
         }
     }
 }

http-clients/apache5-client/src/main/java/software/amazon/awssdk/http/apache5/internal/conn/ClientConnectionManagerFactory.java

@@ -19,10 +19,10 @@
 import java.time.Duration;
 import java.time.Instant;
 import java.util.concurrent.ExecutionException;
-import java.util.concurrent.TimeUnit;
-import org.apache.hc.client5.http.ConnectionPoolTimeoutException;
-import org.apache.hc.client5.http.ConnectionRequest;
-import org.apache.hc.core5.http.io.HttpClientConnection;
+import java.util.concurrent.TimeoutException;
+import org.apache.hc.client5.http.io.ConnectionEndpoint;
+import org.apache.hc.client5.http.io.LeaseRequest;
+import org.apache.hc.core5.util.Timeout;
 import software.amazon.awssdk.annotations.SdkInternalApi;
 import software.amazon.awssdk.http.HttpMetric;
 import software.amazon.awssdk.http.apache5.Apache5HttpClient;
@@ -40,61 +40,61 @@ private ClientConnectionRequestFactory() {
     }
 
     /**
-     * Returns a wrapped instance of {@link ConnectionRequest}
+     * Returns a wrapped instance of {@link LeaseRequest}
      * to capture the necessary performance metrics.
      *
      * @param orig the target instance to be wrapped
      */
-    static ConnectionRequest wrap(ConnectionRequest orig) {
+    static LeaseRequest wrap(LeaseRequest orig) {
         if (orig instanceof DelegatingConnectionRequest) {
             throw new IllegalArgumentException();
         }
         return new InstrumentedConnectionRequest(orig);
     }
 
     /**
-     * Measures the latency of {@link ConnectionRequest#get(long, TimeUnit)}.
+     * Measures the latency of {@link LeaseRequest#get(Timeout)}.
      */
     private static class InstrumentedConnectionRequest extends DelegatingConnectionRequest {
 
-        private InstrumentedConnectionRequest(ConnectionRequest delegate) {
+        private InstrumentedConnectionRequest(LeaseRequest delegate) {
             super(delegate);
         }
 
+
         @Override
-        public HttpClientConnection get(long timeout, TimeUnit timeUnit) throws InterruptedException, ExecutionException,
-                                                                                ConnectionPoolTimeoutException {
+        public ConnectionEndpoint get(Timeout timeout) throws InterruptedException, ExecutionException, TimeoutException {
             Instant startTime = Instant.now();
             try {
-                return super.get(timeout, timeUnit);
+                return super.get(timeout);
             } finally {
                 Duration elapsed = Duration.between(startTime, Instant.now());
                 MetricCollector metricCollector = THREAD_LOCAL_REQUEST_METRIC_COLLECTOR.get();
                 metricCollector.reportMetric(HttpMetric.CONCURRENCY_ACQUIRE_DURATION, elapsed);
             }
         }
+
     }
 
     /**
-     * Delegates all methods to {@link ConnectionRequest}. Subclasses can override select methods to change behavior.
+     * Delegates all methods to {@link LeaseRequest}. Subclasses can override select methods to change behavior.
      */
-    private static class DelegatingConnectionRequest implements ConnectionRequest {
+    private static class DelegatingConnectionRequest implements LeaseRequest {
 
-        private final ConnectionRequest delegate;
+        private final LeaseRequest delegate;
 
-        private DelegatingConnectionRequest(ConnectionRequest delegate) {
+        private DelegatingConnectionRequest(LeaseRequest delegate) {
             this.delegate = delegate;
         }
 
         @Override
-        public HttpClientConnection get(long timeout, TimeUnit timeUnit)
-                throws InterruptedException, ExecutionException, ConnectionPoolTimeoutException {
-            return delegate.get(timeout, timeUnit);
+        public boolean cancel() {
+            return delegate.cancel();
         }
 
         @Override
-        public boolean cancel() {
-            return delegate.cancel();
+        public ConnectionEndpoint get(Timeout timeout) throws InterruptedException, ExecutionException, TimeoutException {
+            return delegate.get(timeout);
         }
     }
 }