From 3baa142191ab61b0949fd0c63edba52c25fe458c Mon Sep 17 00:00:00 2001 From: Istvan Toth Date: Fri, 31 Jan 2025 09:02:04 +0100 Subject: [PATCH 1/2] [CALCITE-6811] Refactor deprecated httpclient API usage in Avatica --- .../remote/AvaticaCommonsHttpClientImpl.java | 31 ++++++++++--- .../remote/CommonsHttpClientPoolCache.java | 45 ++++++------------- .../AvaticaCommonsHttpClientImplTest.java | 14 +++--- 3 files changed, 45 insertions(+), 45 deletions(-) diff --git a/core/src/main/java/org/apache/calcite/avatica/remote/AvaticaCommonsHttpClientImpl.java b/core/src/main/java/org/apache/calcite/avatica/remote/AvaticaCommonsHttpClientImpl.java index 44beaa7b84..f3278908ca 100644 --- a/core/src/main/java/org/apache/calcite/avatica/remote/AvaticaCommonsHttpClientImpl.java +++ b/core/src/main/java/org/apache/calcite/avatica/remote/AvaticaCommonsHttpClientImpl.java @@ -36,16 +36,17 @@ import org.apache.hc.client5.http.impl.auth.DigestSchemeFactory; import org.apache.hc.client5.http.impl.auth.SPNegoSchemeFactory; import org.apache.hc.client5.http.impl.classic.CloseableHttpClient; -import org.apache.hc.client5.http.impl.classic.CloseableHttpResponse; import org.apache.hc.client5.http.impl.classic.HttpClientBuilder; import org.apache.hc.client5.http.impl.classic.HttpClients; import org.apache.hc.client5.http.impl.io.PoolingHttpClientConnectionManager; import org.apache.hc.client5.http.protocol.HttpClientContext; -import org.apache.hc.client5.http.socket.ConnectionSocketFactory; +import org.apache.hc.client5.http.routing.RoutingSupport; +import org.apache.hc.core5.http.ClassicHttpResponse; import org.apache.hc.core5.http.ContentType; +import org.apache.hc.core5.http.HttpException; +import org.apache.hc.core5.http.HttpHost; import org.apache.hc.core5.http.NoHttpResponseException; import org.apache.hc.core5.http.config.Lookup; -import org.apache.hc.core5.http.config.Registry; import org.apache.hc.core5.http.config.RegistryBuilder; import org.apache.hc.core5.http.io.entity.ByteArrayEntity; import org.apache.hc.core5.http.io.entity.EntityUtils; @@ -84,9 +85,9 @@ public class AvaticaCommonsHttpClientImpl implements AvaticaHttpClient, HttpClie private static AuthScope anyAuthScope = new AuthScope(null, -1); protected final URI uri; + protected HttpHost httpHost; protected BasicAuthCache authCache; protected CloseableHttpClient client; - protected Registry socketFactoryRegistry; protected PoolingHttpClientConnectionManager pool; protected UsernamePasswordCredentials credentials = null; @@ -130,6 +131,8 @@ protected void initializeClient(PoolingHttpClientConnectionManager pool, private RequestConfig createRequestConfig() { RequestConfig.Builder requestConfigBuilder = RequestConfig.custom(); requestConfigBuilder + // We cannot avoid this. If the timeout were defined on the pool, then + // it couldn't be overridden later .setConnectTimeout(this.connectTimeout, TimeUnit.MILLISECONDS) .setResponseTimeout(this.responseTimeout, TimeUnit.MILLISECONDS); List preferredSchemes = new ArrayList<>(); @@ -151,6 +154,13 @@ private RequestConfig createRequestConfig() { } @Override public byte[] send(byte[] request) { + try { + // Doing this earlier would break API backwards compatibility + determineHost(); + } catch (HttpException e) { + LOG.debug("Failed to execute HTTP request", e); + throw new RuntimeException("Could not determine Http Host from URI", e); + } while (true) { ByteArrayEntity entity = new ByteArrayEntity(request, ContentType.APPLICATION_OCTET_STREAM); @@ -158,7 +168,7 @@ private RequestConfig createRequestConfig() { HttpPost post = new HttpPost(uri); post.setEntity(entity); - try (CloseableHttpResponse response = execute(post, context)) { + try (ClassicHttpResponse response = executeOpen(httpHost, post, context)) { final int statusCode = response.getCode(); if (HttpURLConnection.HTTP_OK == statusCode || HttpURLConnection.HTTP_INTERNAL_ERROR == statusCode) { @@ -184,10 +194,17 @@ private RequestConfig createRequestConfig() { } } + private void determineHost() throws HttpException { + if (httpHost == null) { + HttpPost dummy = new HttpPost(uri); + this.httpHost = RoutingSupport.determineHost(dummy); + } + } + // Visible for testing - CloseableHttpResponse execute(HttpPost post, HttpClientContext context) + ClassicHttpResponse executeOpen(HttpHost httpHost, HttpPost post, HttpClientContext context) throws IOException, ClientProtocolException { - return client.execute(post, context); + return client.executeOpen(httpHost, post, context); } @Override public void setUsernamePassword(AuthenticationType authType, String username, diff --git a/core/src/main/java/org/apache/calcite/avatica/remote/CommonsHttpClientPoolCache.java b/core/src/main/java/org/apache/calcite/avatica/remote/CommonsHttpClientPoolCache.java index b0821c1b22..c2a304f825 100644 --- a/core/src/main/java/org/apache/calcite/avatica/remote/CommonsHttpClientPoolCache.java +++ b/core/src/main/java/org/apache/calcite/avatica/remote/CommonsHttpClientPoolCache.java @@ -20,13 +20,11 @@ import org.apache.calcite.avatica.remote.HostnameVerificationConfigurable.HostnameVerification; import org.apache.hc.client5.http.impl.io.PoolingHttpClientConnectionManager; -import org.apache.hc.client5.http.socket.ConnectionSocketFactory; -import org.apache.hc.client5.http.socket.PlainConnectionSocketFactory; +import org.apache.hc.client5.http.impl.io.PoolingHttpClientConnectionManagerBuilder; +import org.apache.hc.client5.http.ssl.DefaultClientTlsStrategy; import org.apache.hc.client5.http.ssl.HttpsSupport; import org.apache.hc.client5.http.ssl.NoopHostnameVerifier; -import org.apache.hc.client5.http.ssl.SSLConnectionSocketFactory; -import org.apache.hc.core5.http.config.Registry; -import org.apache.hc.core5.http.config.RegistryBuilder; +import org.apache.hc.client5.http.ssl.TlsSocketStrategy; import org.apache.hc.core5.ssl.SSLContextBuilder; import org.apache.hc.core5.ssl.SSLContexts; @@ -71,36 +69,24 @@ public static PoolingHttpClientConnectionManager getPool(ConnectionConfig config } private static PoolingHttpClientConnectionManager setupPool(ConnectionConfig config) { - Registry csfr = createCSFRegistry(config); - PoolingHttpClientConnectionManager pool = new PoolingHttpClientConnectionManager(csfr); - final String maxCnxns = - System.getProperty(MAX_POOLED_CONNECTIONS_KEY, MAX_POOLED_CONNECTIONS_DEFAULT); - pool.setMaxTotal(Integer.parseInt(maxCnxns)); - // Increase default max connection per route to 25 + final String maxCnxns = System.getProperty(MAX_POOLED_CONNECTIONS_KEY, + MAX_POOLED_CONNECTIONS_DEFAULT); final String maxCnxnsPerRoute = System.getProperty(MAX_POOLED_CONNECTION_PER_ROUTE_KEY, MAX_POOLED_CONNECTION_PER_ROUTE_DEFAULT); - pool.setDefaultMaxPerRoute(Integer.parseInt(maxCnxnsPerRoute)); + PoolingHttpClientConnectionManager pool = PoolingHttpClientConnectionManagerBuilder.create() + .setTlsSocketStrategy(createTlsSocketStrategy(config)) + .setMaxConnTotal(Integer.parseInt(maxCnxns)) + .setMaxConnPerRoute(Integer.parseInt(maxCnxnsPerRoute)).build(); LOG.debug("Created new pool {}", pool); return pool; } - private static Registry createCSFRegistry(ConnectionConfig config) { - RegistryBuilder registryBuilder = RegistryBuilder.create(); - configureHttpRegistry(registryBuilder); - configureHttpsRegistry(registryBuilder, config); - - return registryBuilder.build(); - } - - private static void configureHttpsRegistry( - RegistryBuilder registryBuilder, ConnectionConfig config) { + private static TlsSocketStrategy createTlsSocketStrategy(ConnectionConfig config) { try { - SSLContext sslContext = getSSLContext(config); - final HostnameVerifier verifier = getHostnameVerifier(config.hostnameVerification()); - SSLConnectionSocketFactory sslFactory = new SSLConnectionSocketFactory(sslContext, verifier); - registryBuilder.register("https", sslFactory); + return new DefaultClientTlsStrategy(getSSLContext(config), + getHostnameVerifier(config.hostnameVerification())); } catch (Exception e) { - LOG.error("HTTPS registry configuration failed"); + LOG.error("HTTPS TlsSocketStrategy configuration failed"); throw new RuntimeException(e); } } @@ -134,11 +120,6 @@ private static void loadTrustStore(SSLContextBuilder sslContextBuilder, Connecti LOG.info("Trustore loaded from: {}", config.truststore()); } - private static void configureHttpRegistry( - RegistryBuilder registryBuilder) { - registryBuilder.register("http", PlainConnectionSocketFactory.getSocketFactory()); - } - /** * Creates the {@code HostnameVerifier} given the provided {@code verification}. * diff --git a/core/src/test/java/org/apache/calcite/avatica/remote/AvaticaCommonsHttpClientImplTest.java b/core/src/test/java/org/apache/calcite/avatica/remote/AvaticaCommonsHttpClientImplTest.java index 2b89f16e26..a0247e1f4f 100644 --- a/core/src/test/java/org/apache/calcite/avatica/remote/AvaticaCommonsHttpClientImplTest.java +++ b/core/src/test/java/org/apache/calcite/avatica/remote/AvaticaCommonsHttpClientImplTest.java @@ -22,6 +22,7 @@ import org.apache.hc.client5.http.classic.methods.HttpPost; import org.apache.hc.client5.http.impl.classic.CloseableHttpResponse; import org.apache.hc.client5.http.impl.io.PoolingHttpClientConnectionManager; +import org.apache.hc.core5.http.HttpHost; import org.apache.hc.core5.http.NoHttpResponseException; import org.apache.hc.core5.http.io.entity.ByteArrayEntity; import org.apache.hc.core5.http.io.entity.StringEntity; @@ -76,7 +77,7 @@ public class AvaticaCommonsHttpClientImplTest { ConnectionConfig.class)); doAnswer(failThenSucceed).when(client) - .execute(any(HttpPost.class), eq(client.context)); + .executeOpen(any(HttpHost.class), any(HttpPost.class), eq(client.context)); when(badResponse.getCode()).thenReturn(HttpURLConnection.HTTP_UNAVAILABLE); @@ -110,7 +111,7 @@ public class AvaticaCommonsHttpClientImplTest { ConnectionConfig.class)); doAnswer(failThenSucceed).when(client) - .execute(any(HttpPost.class), eq(client.context)); + .executeOpen(any(HttpHost.class), any(HttpPost.class), eq(client.context)); when(badResponse.getCode()).thenReturn(HttpURLConnection.HTTP_UNAVAILABLE); @@ -136,13 +137,13 @@ public void testPersistentContextReusedAcrossRequests() throws Exception { when(response.getEntity()).thenReturn(entity); doReturn(response).when(client) - .execute(any(HttpPost.class), eq(client.context)); + .executeOpen(any(HttpHost.class), any(HttpPost.class), eq(client.context)); client.send(new byte[0]); client.send(new byte[0]); // Verify that the persistent context was reused and not created again - verify(client, times(2)).execute(any(HttpPost.class), + verify(client, times(2)).executeOpen(any(HttpHost.class), any(HttpPost.class), eq(client.context)); } @@ -154,7 +155,7 @@ public void testPersistentContextThreadSafety() throws Exception { ConnectionConfig.class)); doReturn(mock(CloseableHttpResponse.class)).when(client) - .execute(any(HttpPost.class), eq(client.context)); + .executeOpen(any(HttpHost.class), any(HttpPost.class), eq(client.context)); Runnable requestTask = () -> { try { @@ -175,7 +176,8 @@ public void testPersistentContextThreadSafety() throws Exception { thread.join(); } - verify(client, times(threadCount)).execute(any(HttpPost.class), eq(client.context)); + verify(client, times(threadCount)).executeOpen(any(HttpHost.class), any(HttpPost.class), + eq(client.context)); } } From 656fbaa672aa37db538eea9cd852f04b639cecfb Mon Sep 17 00:00:00 2001 From: Istvan Toth Date: Mon, 10 Feb 2025 19:44:52 +0100 Subject: [PATCH 2/2] avoid dummy Post object for determining httpHost --- .../remote/AvaticaCommonsHttpClientImpl.java | 25 +++++++------------ 1 file changed, 9 insertions(+), 16 deletions(-) diff --git a/core/src/main/java/org/apache/calcite/avatica/remote/AvaticaCommonsHttpClientImpl.java b/core/src/main/java/org/apache/calcite/avatica/remote/AvaticaCommonsHttpClientImpl.java index f3278908ca..7b4cb74b57 100644 --- a/core/src/main/java/org/apache/calcite/avatica/remote/AvaticaCommonsHttpClientImpl.java +++ b/core/src/main/java/org/apache/calcite/avatica/remote/AvaticaCommonsHttpClientImpl.java @@ -154,20 +154,20 @@ private RequestConfig createRequestConfig() { } @Override public byte[] send(byte[] request) { - try { - // Doing this earlier would break API backwards compatibility - determineHost(); - } catch (HttpException e) { - LOG.debug("Failed to execute HTTP request", e); - throw new RuntimeException("Could not determine Http Host from URI", e); - } while (true) { ByteArrayEntity entity = new ByteArrayEntity(request, ContentType.APPLICATION_OCTET_STREAM); - - // Create the client with the AuthSchemeRegistry and manager HttpPost post = new HttpPost(uri); post.setEntity(entity); + if (httpHost == null) { + try { + httpHost = RoutingSupport.determineHost(post); + } catch (HttpException e) { + LOG.debug("Failed to execute HTTP request", e); + throw new RuntimeException("Could not determine Http Host from URI", e); + } + } + try (ClassicHttpResponse response = executeOpen(httpHost, post, context)) { final int statusCode = response.getCode(); if (HttpURLConnection.HTTP_OK == statusCode @@ -194,13 +194,6 @@ private RequestConfig createRequestConfig() { } } - private void determineHost() throws HttpException { - if (httpHost == null) { - HttpPost dummy = new HttpPost(uri); - this.httpHost = RoutingSupport.determineHost(dummy); - } - } - // Visible for testing ClassicHttpResponse executeOpen(HttpHost httpHost, HttpPost post, HttpClientContext context) throws IOException, ClientProtocolException {