nastra commented on code in PR #10053:
URL: https://github.com/apache/iceberg/pull/10053#discussion_r1542440161
##########
core/src/main/java/org/apache/iceberg/rest/HTTPClient.java:
##########
@@ -448,6 +443,43 @@ static HttpRequestInterceptor loadInterceptorDynamically(
return instance;
}
+ static HttpClientConnectionManager getConnectionManager(
+ Map<String, String> properties, ConnectionConfig connectionConfig) {
+ PoolingHttpClientConnectionManagerBuilder connectionManagerBuilder =
+ PoolingHttpClientConnectionManagerBuilder.create();
+ if (connectionConfig != null) {
+ connectionManagerBuilder.setDefaultConnectionConfig(connectionConfig);
+ }
+ return connectionManagerBuilder
+ .useSystemProperties()
+ .setMaxConnTotal(Integer.getInteger(REST_MAX_CONNECTIONS,
REST_MAX_CONNECTIONS_DEFAULT))
+ .setMaxConnPerRoute(
+ PropertyUtil.propertyAsInt(
+ properties, REST_MAX_CONNECTIONS_PER_ROUTE,
REST_MAX_CONNECTIONS_PER_ROUTE_DEFAULT))
+ .build();
+ }
+
+ @VisibleForTesting
+ static ConnectionConfig getConnectionConfig(Map<String, String> properties) {
+ Long connectionTimeout =
+ PropertyUtil.propertyAsNullableLong(properties,
REST_CONNECTION_TIMEOUT);
+ Integer socketTimeout = PropertyUtil.propertyAsNullableInt(properties,
REST_SOCKET_TIMEOUT);
Review Comment:
why is one Long and the other Integer?
##########
core/src/main/java/org/apache/iceberg/rest/HTTPClient.java:
##########
@@ -448,6 +443,43 @@ static HttpRequestInterceptor loadInterceptorDynamically(
return instance;
}
+ static HttpClientConnectionManager getConnectionManager(
+ Map<String, String> properties, ConnectionConfig connectionConfig) {
+ PoolingHttpClientConnectionManagerBuilder connectionManagerBuilder =
+ PoolingHttpClientConnectionManagerBuilder.create();
+ if (connectionConfig != null) {
+ connectionManagerBuilder.setDefaultConnectionConfig(connectionConfig);
+ }
+ return connectionManagerBuilder
+ .useSystemProperties()
+ .setMaxConnTotal(Integer.getInteger(REST_MAX_CONNECTIONS,
REST_MAX_CONNECTIONS_DEFAULT))
+ .setMaxConnPerRoute(
+ PropertyUtil.propertyAsInt(
+ properties, REST_MAX_CONNECTIONS_PER_ROUTE,
REST_MAX_CONNECTIONS_PER_ROUTE_DEFAULT))
+ .build();
+ }
+
+ @VisibleForTesting
+ static ConnectionConfig getConnectionConfig(Map<String, String> properties) {
+ Long connectionTimeout =
+ PropertyUtil.propertyAsNullableLong(properties,
REST_CONNECTION_TIMEOUT);
+ Integer socketTimeout = PropertyUtil.propertyAsNullableInt(properties,
REST_SOCKET_TIMEOUT);
+
+ if (connectionTimeout == null && socketTimeout == null) {
+ return null;
+ }
+
+ ConnectionConfig.Builder connConfigBuilder = ConnectionConfig.custom();
+
+ if (connectionTimeout != null) {
+ connConfigBuilder.setConnectTimeout(connectionTimeout, TimeUnit.SECONDS);
+ }
+ if (socketTimeout != null) {
+ connConfigBuilder.setSocketTimeout(socketTimeout, TimeUnit.SECONDS);
+ }
+ return connConfigBuilder.build();
Review Comment:
same as above
##########
core/src/main/java/org/apache/iceberg/rest/HTTPClient.java:
##########
@@ -493,7 +525,15 @@ public HTTPClient build() {
interceptor =
loadInterceptorDynamically(SIGV4_REQUEST_INTERCEPTOR_IMPL, properties);
}
- return new HTTPClient(uri, baseHeaders, mapper, interceptor, properties);
+ ConnectionConfig connectionConfig = getConnectionConfig(properties);
Review Comment:
can be inlined
##########
core/src/test/java/org/apache/iceberg/rest/TestHTTPClient.java:
##########
@@ -133,6 +136,49 @@ public void testDynamicHttpRequestInterceptorLoading() {
assertThat(((TestHttpRequestInterceptor)
interceptor).properties).isEqualTo(properties);
}
+ @Test
+ public void testHttpClientGetConnectionConfig() {
+ long connectionTimeout = 10L;
+ int socketTimeout = 10;
+ Map<String, String> properties =
+ ImmutableMap.of(
+ HTTPClient.REST_CONNECTION_TIMEOUT,
String.valueOf(connectionTimeout),
+ HTTPClient.REST_SOCKET_TIMEOUT, String.valueOf(socketTimeout));
+
+ ConnectionConfig connectionConfig =
HTTPClient.getConnectionConfig(properties);
+ assertThat(connectionConfig).isNotNull();
+
assertThat(connectionConfig.getConnectTimeout().getDuration()).isEqualTo(connectionTimeout);
+
assertThat(connectionConfig.getSocketTimeout().getDuration()).isEqualTo(socketTimeout);
+ }
+
+ @Test
+ public void testHttpClientWithSocketTimeout() throws IOException {
+ long socketTimeoutSec = 2L;
+ Map<String, String> properties =
+ ImmutableMap.of(HTTPClient.REST_SOCKET_TIMEOUT,
String.valueOf(socketTimeoutSec));
+ String path = "socket/timeout/path";
+
+ try (HTTPClient client = HTTPClient.builder(properties).uri(URI).build()) {
+
Review Comment:
nit: unnecessary empty line
##########
core/src/main/java/org/apache/iceberg/rest/HTTPClient.java:
##########
@@ -448,6 +443,43 @@ static HttpRequestInterceptor loadInterceptorDynamically(
return instance;
}
+ static HttpClientConnectionManager getConnectionManager(
+ Map<String, String> properties, ConnectionConfig connectionConfig) {
+ PoolingHttpClientConnectionManagerBuilder connectionManagerBuilder =
+ PoolingHttpClientConnectionManagerBuilder.create();
+ if (connectionConfig != null) {
+ connectionManagerBuilder.setDefaultConnectionConfig(connectionConfig);
+ }
+ return connectionManagerBuilder
+ .useSystemProperties()
+ .setMaxConnTotal(Integer.getInteger(REST_MAX_CONNECTIONS,
REST_MAX_CONNECTIONS_DEFAULT))
+ .setMaxConnPerRoute(
+ PropertyUtil.propertyAsInt(
+ properties, REST_MAX_CONNECTIONS_PER_ROUTE,
REST_MAX_CONNECTIONS_PER_ROUTE_DEFAULT))
+ .build();
+ }
+
+ @VisibleForTesting
+ static ConnectionConfig getConnectionConfig(Map<String, String> properties) {
+ Long connectionTimeout =
+ PropertyUtil.propertyAsNullableLong(properties,
REST_CONNECTION_TIMEOUT);
+ Integer socketTimeout = PropertyUtil.propertyAsNullableInt(properties,
REST_SOCKET_TIMEOUT);
+
+ if (connectionTimeout == null && socketTimeout == null) {
+ return null;
+ }
+
+ ConnectionConfig.Builder connConfigBuilder = ConnectionConfig.custom();
+
+ if (connectionTimeout != null) {
+ connConfigBuilder.setConnectTimeout(connectionTimeout, TimeUnit.SECONDS);
+ }
+ if (socketTimeout != null) {
Review Comment:
nit: please add newline after closing }
##########
core/src/test/java/org/apache/iceberg/rest/TestHTTPClient.java:
##########
@@ -133,6 +136,49 @@ public void testDynamicHttpRequestInterceptorLoading() {
assertThat(((TestHttpRequestInterceptor)
interceptor).properties).isEqualTo(properties);
}
+ @Test
+ public void testHttpClientGetConnectionConfig() {
+ long connectionTimeout = 10L;
+ int socketTimeout = 10;
+ Map<String, String> properties =
+ ImmutableMap.of(
+ HTTPClient.REST_CONNECTION_TIMEOUT,
String.valueOf(connectionTimeout),
Review Comment:
there should probably also be a test where only one of these is set. Also
please add some (separate) tests where connection/socket timeout are being set
to some random string
##########
core/src/main/java/org/apache/iceberg/rest/HTTPClient.java:
##########
@@ -448,6 +443,43 @@ static HttpRequestInterceptor loadInterceptorDynamically(
return instance;
}
+ static HttpClientConnectionManager getConnectionManager(
Review Comment:
the codebase avoids using `get` as a prefix, so I think
`configureConnectionManager` might make more sense here
##########
core/src/main/java/org/apache/iceberg/rest/HTTPClient.java:
##########
@@ -78,6 +80,8 @@ public class HTTPClient implements RESTClient {
private static final int REST_MAX_CONNECTIONS_DEFAULT = 100;
private static final String REST_MAX_CONNECTIONS_PER_ROUTE =
"rest.client.connections-per-route";
private static final int REST_MAX_CONNECTIONS_PER_ROUTE_DEFAULT = 100;
+ @VisibleForTesting static final String REST_CONNECTION_TIMEOUT =
"rest.client.connection-timeout";
+ @VisibleForTesting static final String REST_SOCKET_TIMEOUT =
"rest.client.socket-timeout";
Review Comment:
are those set in millis or in seconds? In other places we always indicate in
the property naming whether it's millis or not ->
`rest.client.socket-timeout-ms`
--
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.
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
