You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "harishch1998 (via GitHub)" <gi...@apache.org> on 2024/03/28 01:18:42 UTC
[PR] Introduce two properties for reading the connection timeout and socke… [iceberg]
harishch1998 opened a new pull request, #10053:
URL: https://github.com/apache/iceberg/pull/10053
This PR includes code changes to make the REST HTTPClient's socket and connection timeout values configurable. Introducing two new properties to help achieve this.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Introduce two properties for reading the connection timeout and socke… [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10053:
URL: https://github.com/apache/iceberg/pull/10053#discussion_r1547299122
##########
core/src/test/java/org/apache/iceberg/rest/TestHTTPClient.java:
##########
@@ -133,6 +136,59 @@ public void testDynamicHttpRequestInterceptorLoading() {
assertThat(((TestHttpRequestInterceptor) interceptor).properties).isEqualTo(properties);
}
+ @Test
+ public void testHttpClientGetConnectionConfig() {
Review Comment:
```suggestion
public void testSocketAndConnectionTimeoutSet() {
```
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Introduce two properties for reading the connection timeout and socke… [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10053:
URL: https://github.com/apache/iceberg/pull/10053#discussion_r1547305188
##########
core/src/test/java/org/apache/iceberg/rest/TestHTTPClient.java:
##########
@@ -133,6 +136,59 @@ public void testDynamicHttpRequestInterceptorLoading() {
assertThat(((TestHttpRequestInterceptor) interceptor).properties).isEqualTo(properties);
}
+ @Test
+ public void testHttpClientGetConnectionConfig() {
+ long connectionTimeoutMs = 10L;
+ int socketTimeoutMs = 10;
+ Map<String, String> properties =
+ ImmutableMap.of(
+ HTTPClient.REST_CONNECTION_TIMEOUT_MS, String.valueOf(connectionTimeoutMs),
+ HTTPClient.REST_SOCKET_TIMEOUT_MS, String.valueOf(socketTimeoutMs));
+
+ ConnectionConfig connectionConfig = HTTPClient.configureConnectionConfig(properties);
+ assertThat(connectionConfig).isNotNull();
+ assertThat(connectionConfig.getConnectTimeout().getDuration()).isEqualTo(connectionTimeoutMs);
+ assertThat(connectionConfig.getSocketTimeout().getDuration()).isEqualTo(socketTimeoutMs);
+ }
+
+ @Test
+ public void testHttpClientWithSocketTimeout() throws IOException {
Review Comment:
should there also be a test verifying the connection timeout?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Introduce two properties for reading the connection timeout and socke… [iceberg]
Posted by "harishch1998 (via GitHub)" <gi...@apache.org>.
harishch1998 commented on code in PR #10053:
URL: https://github.com/apache/iceberg/pull/10053#discussion_r1554368145
##########
core/src/test/java/org/apache/iceberg/rest/TestHTTPClient.java:
##########
@@ -133,6 +136,59 @@ public void testDynamicHttpRequestInterceptorLoading() {
assertThat(((TestHttpRequestInterceptor) interceptor).properties).isEqualTo(properties);
}
+ @Test
+ public void testHttpClientGetConnectionConfig() {
+ long connectionTimeoutMs = 10L;
+ int socketTimeoutMs = 10;
+ Map<String, String> properties =
+ ImmutableMap.of(
+ HTTPClient.REST_CONNECTION_TIMEOUT_MS, String.valueOf(connectionTimeoutMs),
+ HTTPClient.REST_SOCKET_TIMEOUT_MS, String.valueOf(socketTimeoutMs));
+
+ ConnectionConfig connectionConfig = HTTPClient.configureConnectionConfig(properties);
+ assertThat(connectionConfig).isNotNull();
+ assertThat(connectionConfig.getConnectTimeout().getDuration()).isEqualTo(connectionTimeoutMs);
+ assertThat(connectionConfig.getSocketTimeout().getDuration()).isEqualTo(socketTimeoutMs);
+ }
+
+ @Test
+ public void testHttpClientWithSocketTimeout() throws IOException {
Review Comment:
I tried multiple solutions but couldn't specify a custom connection timeout on the mockServer. It seems like a network-level configuration and is non-trivial to unit test with the existing MockServer that we use in testing.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Introduce two properties for reading the connection timeout and socke… [iceberg]
Posted by "harishch1998 (via GitHub)" <gi...@apache.org>.
harishch1998 commented on code in PR #10053:
URL: https://github.com/apache/iceberg/pull/10053#discussion_r1544095303
##########
core/src/main/java/org/apache/iceberg/rest/HTTPClient.java:
##########
@@ -448,6 +443,43 @@ static HttpRequestInterceptor loadInterceptorDynamically(
return instance;
}
+ static HttpClientConnectionManager getConnectionManager(
Review Comment:
Thanks for catching this. Just saw this in the [coding guidelines](https://iceberg.apache.org/contribute/#method-naming) as well.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Introduce two properties for reading the connection timeout and socke… [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10053:
URL: https://github.com/apache/iceberg/pull/10053#discussion_r1547300740
##########
core/src/test/java/org/apache/iceberg/rest/TestHTTPClient.java:
##########
@@ -133,6 +136,59 @@ public void testDynamicHttpRequestInterceptorLoading() {
assertThat(((TestHttpRequestInterceptor) interceptor).properties).isEqualTo(properties);
}
+ @Test
+ public void testHttpClientGetConnectionConfig() {
+ long connectionTimeoutMs = 10L;
+ int socketTimeoutMs = 10;
+ Map<String, String> properties =
+ ImmutableMap.of(
+ HTTPClient.REST_CONNECTION_TIMEOUT_MS, String.valueOf(connectionTimeoutMs),
+ HTTPClient.REST_SOCKET_TIMEOUT_MS, String.valueOf(socketTimeoutMs));
+
+ ConnectionConfig connectionConfig = HTTPClient.configureConnectionConfig(properties);
+ assertThat(connectionConfig).isNotNull();
+ assertThat(connectionConfig.getConnectTimeout().getDuration()).isEqualTo(connectionTimeoutMs);
+ assertThat(connectionConfig.getSocketTimeout().getDuration()).isEqualTo(socketTimeoutMs);
+ }
+
+ @Test
+ public void testHttpClientWithSocketTimeout() throws IOException {
+ long socketTimeoutMs = 2000L;
+ Map<String, String> properties =
+ ImmutableMap.of(HTTPClient.REST_SOCKET_TIMEOUT_MS, String.valueOf(socketTimeoutMs));
+ String path = "socket/timeout/path";
+
+ try (HTTPClient client = HTTPClient.builder(properties).uri(URI).build()) {
+ HttpRequest mockRequest =
+ request()
+ .withPath("/" + path)
+ .withMethod(HttpMethod.HEAD.name().toUpperCase(Locale.ROOT));
+ // Setting a response delay of 5 seconds to simulate hitting the configured socket timeout of
+ // 2 seconds
+ HttpResponse mockResponse =
+ response()
+ .withStatusCode(200)
+ .withBody("Delayed response")
+ .withDelay(TimeUnit.MILLISECONDS, 5000);
+ mockServer.when(mockRequest).respond(mockResponse);
+
+ Assertions.assertThatThrownBy(() -> client.head(path, ImmutableMap.of(), (unused) -> {}))
+ .cause()
+ .isInstanceOf(SocketTimeoutException.class);
+ }
+ }
+
+ @Test
+ public void testHttpClientInvalidConnectionTimeout() {
Review Comment:
what about an invalid socket timeout?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Introduce two properties for reading the connection timeout and socke… [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10053:
URL: https://github.com/apache/iceberg/pull/10053#discussion_r1547287402
##########
core/src/main/java/org/apache/iceberg/rest/HTTPClient.java:
##########
@@ -448,6 +446,47 @@ static HttpRequestInterceptor loadInterceptorDynamically(
return instance;
}
+ static HttpClientConnectionManager configureConnectionManager(
+ 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 configureConnectionConfig(Map<String, String> properties) {
+ Long connectionTimeout =
+ PropertyUtil.propertyAsNullableLong(properties, REST_CONNECTION_TIMEOUT_MS);
+ Integer socketTimeout = PropertyUtil.propertyAsNullableInt(properties, REST_SOCKET_TIMEOUT_MS);
Review Comment:
```suggestion
Integer socketTimeoutMillis = PropertyUtil.propertyAsNullableInt(properties, REST_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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Introduce two properties for reading the connection timeout and socke… [iceberg]
Posted by "harishch1998 (via GitHub)" <gi...@apache.org>.
harishch1998 commented on code in PR #10053:
URL: https://github.com/apache/iceberg/pull/10053#discussion_r1544095514
##########
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:
Yeah, these are milliseconds. I'll fix the naming.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Introduce two properties for reading the connection timeout and socke… [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10053:
URL: https://github.com/apache/iceberg/pull/10053#discussion_r1547301269
##########
core/src/test/java/org/apache/iceberg/rest/TestHTTPClient.java:
##########
@@ -133,6 +136,59 @@ public void testDynamicHttpRequestInterceptorLoading() {
assertThat(((TestHttpRequestInterceptor) interceptor).properties).isEqualTo(properties);
}
+ @Test
+ public void testHttpClientGetConnectionConfig() {
+ long connectionTimeoutMs = 10L;
+ int socketTimeoutMs = 10;
+ Map<String, String> properties =
+ ImmutableMap.of(
+ HTTPClient.REST_CONNECTION_TIMEOUT_MS, String.valueOf(connectionTimeoutMs),
+ HTTPClient.REST_SOCKET_TIMEOUT_MS, String.valueOf(socketTimeoutMs));
+
+ ConnectionConfig connectionConfig = HTTPClient.configureConnectionConfig(properties);
+ assertThat(connectionConfig).isNotNull();
+ assertThat(connectionConfig.getConnectTimeout().getDuration()).isEqualTo(connectionTimeoutMs);
+ assertThat(connectionConfig.getSocketTimeout().getDuration()).isEqualTo(socketTimeoutMs);
+ }
+
+ @Test
+ public void testHttpClientWithSocketTimeout() throws IOException {
+ long socketTimeoutMs = 2000L;
+ Map<String, String> properties =
+ ImmutableMap.of(HTTPClient.REST_SOCKET_TIMEOUT_MS, String.valueOf(socketTimeoutMs));
+ String path = "socket/timeout/path";
+
+ try (HTTPClient client = HTTPClient.builder(properties).uri(URI).build()) {
+ HttpRequest mockRequest =
+ request()
+ .withPath("/" + path)
+ .withMethod(HttpMethod.HEAD.name().toUpperCase(Locale.ROOT));
+ // Setting a response delay of 5 seconds to simulate hitting the configured socket timeout of
+ // 2 seconds
+ HttpResponse mockResponse =
+ response()
+ .withStatusCode(200)
+ .withBody("Delayed response")
+ .withDelay(TimeUnit.MILLISECONDS, 5000);
+ mockServer.when(mockRequest).respond(mockResponse);
+
+ Assertions.assertThatThrownBy(() -> client.head(path, ImmutableMap.of(), (unused) -> {}))
+ .cause()
+ .isInstanceOf(SocketTimeoutException.class);
+ }
+ }
+
+ @Test
+ public void testHttpClientInvalidConnectionTimeout() {
+ // We expect a Runtime exception
+ String connectionTimeoutMsStr = "invalidMs";
+ Map<String, String> properties =
+ ImmutableMap.of(HTTPClient.REST_CONNECTION_TIMEOUT_MS, connectionTimeoutMsStr);
+
+ Assertions.assertThatThrownBy(() -> HTTPClient.builder(properties).uri(URI).build())
+ .isInstanceOf(RuntimeException.class);
Review Comment:
this should have a `.hasMessage(...)` check
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Introduce two properties for reading the connection timeout and socke… [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Introduce two properties for reading the connection timeout and socke… [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10053:
URL: https://github.com/apache/iceberg/pull/10053#discussion_r1547292782
##########
core/src/test/java/org/apache/iceberg/rest/TestHTTPClient.java:
##########
@@ -133,6 +136,59 @@ public void testDynamicHttpRequestInterceptorLoading() {
assertThat(((TestHttpRequestInterceptor) interceptor).properties).isEqualTo(properties);
}
+ @Test
+ public void testHttpClientGetConnectionConfig() {
+ long connectionTimeoutMs = 10L;
Review Comment:
```suggestion
long connectionTimeoutMillis = 10L;
```
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Introduce two properties for reading the connection timeout and socke… [iceberg]
Posted by "harishch1998 (via GitHub)" <gi...@apache.org>.
harishch1998 commented on code in PR #10053:
URL: https://github.com/apache/iceberg/pull/10053#discussion_r1554368145
##########
core/src/test/java/org/apache/iceberg/rest/TestHTTPClient.java:
##########
@@ -133,6 +136,59 @@ public void testDynamicHttpRequestInterceptorLoading() {
assertThat(((TestHttpRequestInterceptor) interceptor).properties).isEqualTo(properties);
}
+ @Test
+ public void testHttpClientGetConnectionConfig() {
+ long connectionTimeoutMs = 10L;
+ int socketTimeoutMs = 10;
+ Map<String, String> properties =
+ ImmutableMap.of(
+ HTTPClient.REST_CONNECTION_TIMEOUT_MS, String.valueOf(connectionTimeoutMs),
+ HTTPClient.REST_SOCKET_TIMEOUT_MS, String.valueOf(socketTimeoutMs));
+
+ ConnectionConfig connectionConfig = HTTPClient.configureConnectionConfig(properties);
+ assertThat(connectionConfig).isNotNull();
+ assertThat(connectionConfig.getConnectTimeout().getDuration()).isEqualTo(connectionTimeoutMs);
+ assertThat(connectionConfig.getSocketTimeout().getDuration()).isEqualTo(socketTimeoutMs);
+ }
+
+ @Test
+ public void testHttpClientWithSocketTimeout() throws IOException {
Review Comment:
I tried multiple solutions but couldn't specify a custom connection timeout on the mockServer. It seems like a network-level configuration and is non-trivial to unit test the behavior with the existing MockServer that we use in testing.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Introduce two properties for reading the connection timeout and socke… [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10053:
URL: https://github.com/apache/iceberg/pull/10053#discussion_r1547304325
##########
core/src/test/java/org/apache/iceberg/rest/TestHTTPClient.java:
##########
@@ -133,6 +136,59 @@ public void testDynamicHttpRequestInterceptorLoading() {
assertThat(((TestHttpRequestInterceptor) interceptor).properties).isEqualTo(properties);
}
+ @Test
+ public void testHttpClientGetConnectionConfig() {
+ long connectionTimeoutMs = 10L;
+ int socketTimeoutMs = 10;
+ Map<String, String> properties =
+ ImmutableMap.of(
+ HTTPClient.REST_CONNECTION_TIMEOUT_MS, String.valueOf(connectionTimeoutMs),
+ HTTPClient.REST_SOCKET_TIMEOUT_MS, String.valueOf(socketTimeoutMs));
+
+ ConnectionConfig connectionConfig = HTTPClient.configureConnectionConfig(properties);
+ assertThat(connectionConfig).isNotNull();
+ assertThat(connectionConfig.getConnectTimeout().getDuration()).isEqualTo(connectionTimeoutMs);
+ assertThat(connectionConfig.getSocketTimeout().getDuration()).isEqualTo(socketTimeoutMs);
+ }
+
+ @Test
+ public void testHttpClientWithSocketTimeout() throws IOException {
+ long socketTimeoutMs = 2000L;
+ Map<String, String> properties =
+ ImmutableMap.of(HTTPClient.REST_SOCKET_TIMEOUT_MS, String.valueOf(socketTimeoutMs));
+ String path = "socket/timeout/path";
+
+ try (HTTPClient client = HTTPClient.builder(properties).uri(URI).build()) {
+ HttpRequest mockRequest =
+ request()
+ .withPath("/" + path)
+ .withMethod(HttpMethod.HEAD.name().toUpperCase(Locale.ROOT));
+ // Setting a response delay of 5 seconds to simulate hitting the configured socket timeout of
+ // 2 seconds
+ HttpResponse mockResponse =
+ response()
+ .withStatusCode(200)
+ .withBody("Delayed response")
+ .withDelay(TimeUnit.MILLISECONDS, 5000);
+ mockServer.when(mockRequest).respond(mockResponse);
+
+ Assertions.assertThatThrownBy(() -> client.head(path, ImmutableMap.of(), (unused) -> {}))
+ .cause()
+ .isInstanceOf(SocketTimeoutException.class);
+ }
+ }
+
+ @Test
+ public void testHttpClientInvalidConnectionTimeout() {
Review Comment:
also no need for the `HttpClient` prefix in the test names as this is already obvious, because the test class itself is called `TestHTTPClient`
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Introduce two properties for reading the connection timeout and socke… [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10053:
URL: https://github.com/apache/iceberg/pull/10053#discussion_r1547287086
##########
core/src/main/java/org/apache/iceberg/rest/HTTPClient.java:
##########
@@ -448,6 +446,47 @@ static HttpRequestInterceptor loadInterceptorDynamically(
return instance;
}
+ static HttpClientConnectionManager configureConnectionManager(
+ 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 configureConnectionConfig(Map<String, String> properties) {
+ Long connectionTimeout =
Review Comment:
```suggestion
Long connectionTimeoutMillis =
```
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Introduce two properties for reading the connection timeout and socke… [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10053:
URL: https://github.com/apache/iceberg/pull/10053#discussion_r1547290305
##########
core/src/main/java/org/apache/iceberg/rest/HTTPClient.java:
##########
@@ -448,6 +446,47 @@ static HttpRequestInterceptor loadInterceptorDynamically(
return instance;
}
+ static HttpClientConnectionManager configureConnectionManager(
+ Map<String, String> properties, ConnectionConfig connectionConfig) {
Review Comment:
rather than passing in the `ConnectionConfig` I would probably just call `configureConnectionConfig(properties)` inside `configureConnectionManager(..)`
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Introduce two properties for reading the connection timeout and socke… [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10053:
URL: https://github.com/apache/iceberg/pull/10053#discussion_r1547292782
##########
core/src/test/java/org/apache/iceberg/rest/TestHTTPClient.java:
##########
@@ -133,6 +136,59 @@ public void testDynamicHttpRequestInterceptorLoading() {
assertThat(((TestHttpRequestInterceptor) interceptor).properties).isEqualTo(properties);
}
+ @Test
+ public void testHttpClientGetConnectionConfig() {
+ long connectionTimeoutMs = 10L;
Review Comment:
```suggestion
long connectionTimeoutMillis = 10L;
```
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Introduce two properties for reading the connection timeout and socke… [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10053:
URL: https://github.com/apache/iceberg/pull/10053#discussion_r1555349405
##########
core/src/test/java/org/apache/iceberg/rest/TestHTTPClient.java:
##########
@@ -133,6 +136,67 @@ public void testDynamicHttpRequestInterceptorLoading() {
assertThat(((TestHttpRequestInterceptor) interceptor).properties).isEqualTo(properties);
}
+ @Test
+ public void testSocketAndConnectionTimeoutSet() {
+ long connectionTimeoutMs = 10L;
+ int socketTimeoutMs = 10;
+ Map<String, String> properties =
+ ImmutableMap.of(
+ HTTPClient.REST_CONNECTION_TIMEOUT_MS, String.valueOf(connectionTimeoutMs),
+ HTTPClient.REST_SOCKET_TIMEOUT_MS, String.valueOf(socketTimeoutMs));
+
+ ConnectionConfig connectionConfig = HTTPClient.configureConnectionConfig(properties);
+ assertThat(connectionConfig).isNotNull();
+ assertThat(connectionConfig.getConnectTimeout().getDuration()).isEqualTo(connectionTimeoutMs);
+ assertThat(connectionConfig.getSocketTimeout().getDuration()).isEqualTo(socketTimeoutMs);
+ }
+
+ @Test
+ public void testSocketTimeout() throws IOException {
+ long socketTimeoutMs = 2000L;
+ Map<String, String> properties =
+ ImmutableMap.of(HTTPClient.REST_SOCKET_TIMEOUT_MS, String.valueOf(socketTimeoutMs));
+ String path = "socket/timeout/path";
+
+ try (HTTPClient client = HTTPClient.builder(properties).uri(URI).build()) {
+ HttpRequest mockRequest =
+ request()
+ .withPath("/" + path)
+ .withMethod(HttpMethod.HEAD.name().toUpperCase(Locale.ROOT));
+ // Setting a response delay of 5 seconds to simulate hitting the configured socket timeout of
+ // 2 seconds
+ HttpResponse mockResponse =
+ response()
+ .withStatusCode(200)
+ .withBody("Delayed response")
+ .withDelay(TimeUnit.MILLISECONDS, 5000);
+ mockServer.when(mockRequest).respond(mockResponse);
+
+ Assertions.assertThatThrownBy(() -> client.head(path, ImmutableMap.of(), (unused) -> {}))
+ .cause()
+ .isInstanceOf(SocketTimeoutException.class);
+ }
+ }
+
+ @Test
+ public void testInvalidConnectionTimeout() {
+ testInvalidTimeouts(HTTPClient.REST_CONNECTION_TIMEOUT_MS);
+ }
+
+ @Test
+ public void testInvalidSocketTimeout() {
+ testInvalidTimeouts(HTTPClient.REST_SOCKET_TIMEOUT_MS);
+ }
+
+ private void testInvalidTimeouts(String timeoutMsType) {
Review Comment:
```suggestion
@ParameterizedTest
@ValueSource(strings = {HTTPClient.REST_CONNECTION_TIMEOUT_MS), HTTPClient.REST_SOCKET_TIMEOUT_MS})
public void testInvalidTimeout(String timeoutType) {
```
##########
core/src/test/java/org/apache/iceberg/rest/TestHTTPClient.java:
##########
@@ -133,6 +136,67 @@ public void testDynamicHttpRequestInterceptorLoading() {
assertThat(((TestHttpRequestInterceptor) interceptor).properties).isEqualTo(properties);
}
+ @Test
+ public void testSocketAndConnectionTimeoutSet() {
+ long connectionTimeoutMs = 10L;
+ int socketTimeoutMs = 10;
+ Map<String, String> properties =
+ ImmutableMap.of(
+ HTTPClient.REST_CONNECTION_TIMEOUT_MS, String.valueOf(connectionTimeoutMs),
+ HTTPClient.REST_SOCKET_TIMEOUT_MS, String.valueOf(socketTimeoutMs));
+
+ ConnectionConfig connectionConfig = HTTPClient.configureConnectionConfig(properties);
+ assertThat(connectionConfig).isNotNull();
+ assertThat(connectionConfig.getConnectTimeout().getDuration()).isEqualTo(connectionTimeoutMs);
+ assertThat(connectionConfig.getSocketTimeout().getDuration()).isEqualTo(socketTimeoutMs);
+ }
+
+ @Test
+ public void testSocketTimeout() throws IOException {
+ long socketTimeoutMs = 2000L;
+ Map<String, String> properties =
+ ImmutableMap.of(HTTPClient.REST_SOCKET_TIMEOUT_MS, String.valueOf(socketTimeoutMs));
+ String path = "socket/timeout/path";
+
+ try (HTTPClient client = HTTPClient.builder(properties).uri(URI).build()) {
+ HttpRequest mockRequest =
+ request()
+ .withPath("/" + path)
+ .withMethod(HttpMethod.HEAD.name().toUpperCase(Locale.ROOT));
+ // Setting a response delay of 5 seconds to simulate hitting the configured socket timeout of
+ // 2 seconds
+ HttpResponse mockResponse =
+ response()
+ .withStatusCode(200)
+ .withBody("Delayed response")
+ .withDelay(TimeUnit.MILLISECONDS, 5000);
+ mockServer.when(mockRequest).respond(mockResponse);
+
+ Assertions.assertThatThrownBy(() -> client.head(path, ImmutableMap.of(), (unused) -> {}))
+ .cause()
+ .isInstanceOf(SocketTimeoutException.class);
+ }
+ }
+
+ @Test
+ public void testInvalidConnectionTimeout() {
+ testInvalidTimeouts(HTTPClient.REST_CONNECTION_TIMEOUT_MS);
+ }
+
+ @Test
+ public void testInvalidSocketTimeout() {
+ testInvalidTimeouts(HTTPClient.REST_SOCKET_TIMEOUT_MS);
+ }
+
+ private void testInvalidTimeouts(String timeoutMsType) {
Review Comment:
you can then remove `testInvalidConnectionTimeout` / `testInvalidSocketTimeout`
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Introduce two properties for reading the connection timeout and socke… [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10053:
URL: https://github.com/apache/iceberg/pull/10053#discussion_r1555528393
##########
core/src/test/java/org/apache/iceberg/rest/TestHTTPClient.java:
##########
@@ -133,6 +138,59 @@ public void testDynamicHttpRequestInterceptorLoading() {
assertThat(((TestHttpRequestInterceptor) interceptor).properties).isEqualTo(properties);
}
+ @Test
+ public void testSocketAndConnectionTimeoutSet() {
+ long connectionTimeoutMs = 10L;
+ int socketTimeoutMs = 10;
+ Map<String, String> properties =
+ ImmutableMap.of(
+ HTTPClient.REST_CONNECTION_TIMEOUT_MS, String.valueOf(connectionTimeoutMs),
+ HTTPClient.REST_SOCKET_TIMEOUT_MS, String.valueOf(socketTimeoutMs));
+
+ ConnectionConfig connectionConfig = HTTPClient.configureConnectionConfig(properties);
+ assertThat(connectionConfig).isNotNull();
+ assertThat(connectionConfig.getConnectTimeout().getDuration()).isEqualTo(connectionTimeoutMs);
+ assertThat(connectionConfig.getSocketTimeout().getDuration()).isEqualTo(socketTimeoutMs);
+ }
+
+ @Test
+ public void testSocketTimeout() throws IOException {
+ long socketTimeoutMs = 2000L;
+ Map<String, String> properties =
+ ImmutableMap.of(HTTPClient.REST_SOCKET_TIMEOUT_MS, String.valueOf(socketTimeoutMs));
+ String path = "socket/timeout/path";
+
+ try (HTTPClient client = HTTPClient.builder(properties).uri(URI).build()) {
+ HttpRequest mockRequest =
+ request()
+ .withPath("/" + path)
+ .withMethod(HttpMethod.HEAD.name().toUpperCase(Locale.ROOT));
+ // Setting a response delay of 5 seconds to simulate hitting the configured socket timeout of
+ // 2 seconds
+ HttpResponse mockResponse =
+ response()
+ .withStatusCode(200)
+ .withBody("Delayed response")
+ .withDelay(TimeUnit.MILLISECONDS, 5000);
+ mockServer.when(mockRequest).respond(mockResponse);
+
+ Assertions.assertThatThrownBy(() -> client.head(path, ImmutableMap.of(), (unused) -> {}))
+ .cause()
+ .isInstanceOf(SocketTimeoutException.class);
+ }
+ }
+
+ @ParameterizedTest
+ @ValueSource(strings = {HTTPClient.REST_CONNECTION_TIMEOUT_MS, HTTPClient.REST_SOCKET_TIMEOUT_MS})
+ public void testInvalidTimeout(String timeoutMsType) {
Review Comment:
can you also please add a test where the timout is -1?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Introduce two properties for reading the connection timeout and socke… [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10053:
URL: https://github.com/apache/iceberg/pull/10053#discussion_r1555525438
##########
core/src/test/java/org/apache/iceberg/rest/TestHTTPClient.java:
##########
@@ -133,6 +138,59 @@ public void testDynamicHttpRequestInterceptorLoading() {
assertThat(((TestHttpRequestInterceptor) interceptor).properties).isEqualTo(properties);
}
+ @Test
+ public void testSocketAndConnectionTimeoutSet() {
+ long connectionTimeoutMs = 10L;
+ int socketTimeoutMs = 10;
+ Map<String, String> properties =
+ ImmutableMap.of(
+ HTTPClient.REST_CONNECTION_TIMEOUT_MS, String.valueOf(connectionTimeoutMs),
+ HTTPClient.REST_SOCKET_TIMEOUT_MS, String.valueOf(socketTimeoutMs));
+
+ ConnectionConfig connectionConfig = HTTPClient.configureConnectionConfig(properties);
+ assertThat(connectionConfig).isNotNull();
+ assertThat(connectionConfig.getConnectTimeout().getDuration()).isEqualTo(connectionTimeoutMs);
+ assertThat(connectionConfig.getSocketTimeout().getDuration()).isEqualTo(socketTimeoutMs);
+ }
+
+ @Test
+ public void testSocketTimeout() throws IOException {
+ long socketTimeoutMs = 2000L;
+ Map<String, String> properties =
+ ImmutableMap.of(HTTPClient.REST_SOCKET_TIMEOUT_MS, String.valueOf(socketTimeoutMs));
+ String path = "socket/timeout/path";
+
+ try (HTTPClient client = HTTPClient.builder(properties).uri(URI).build()) {
+ HttpRequest mockRequest =
+ request()
+ .withPath("/" + path)
+ .withMethod(HttpMethod.HEAD.name().toUpperCase(Locale.ROOT));
+ // Setting a response delay of 5 seconds to simulate hitting the configured socket timeout of
+ // 2 seconds
+ HttpResponse mockResponse =
+ response()
+ .withStatusCode(200)
+ .withBody("Delayed response")
+ .withDelay(TimeUnit.MILLISECONDS, 5000);
+ mockServer.when(mockRequest).respond(mockResponse);
+
+ Assertions.assertThatThrownBy(() -> client.head(path, ImmutableMap.of(), (unused) -> {}))
+ .cause()
+ .isInstanceOf(SocketTimeoutException.class);
Review Comment:
this should also have `.hasMessage("Read timed out")`
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Introduce two properties for reading the connection timeout and socke… [iceberg]
Posted by "harishch1998 (via GitHub)" <gi...@apache.org>.
harishch1998 commented on code in PR #10053:
URL: https://github.com/apache/iceberg/pull/10053#discussion_r1545794426
##########
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:
SocketTimeout is an int type in the apache closeable httpclient whereas connection timeout is a long
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Introduce two properties for reading the connection timeout and socke… [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10053:
URL: https://github.com/apache/iceberg/pull/10053#discussion_r1555349405
##########
core/src/test/java/org/apache/iceberg/rest/TestHTTPClient.java:
##########
@@ -133,6 +136,67 @@ public void testDynamicHttpRequestInterceptorLoading() {
assertThat(((TestHttpRequestInterceptor) interceptor).properties).isEqualTo(properties);
}
+ @Test
+ public void testSocketAndConnectionTimeoutSet() {
+ long connectionTimeoutMs = 10L;
+ int socketTimeoutMs = 10;
+ Map<String, String> properties =
+ ImmutableMap.of(
+ HTTPClient.REST_CONNECTION_TIMEOUT_MS, String.valueOf(connectionTimeoutMs),
+ HTTPClient.REST_SOCKET_TIMEOUT_MS, String.valueOf(socketTimeoutMs));
+
+ ConnectionConfig connectionConfig = HTTPClient.configureConnectionConfig(properties);
+ assertThat(connectionConfig).isNotNull();
+ assertThat(connectionConfig.getConnectTimeout().getDuration()).isEqualTo(connectionTimeoutMs);
+ assertThat(connectionConfig.getSocketTimeout().getDuration()).isEqualTo(socketTimeoutMs);
+ }
+
+ @Test
+ public void testSocketTimeout() throws IOException {
+ long socketTimeoutMs = 2000L;
+ Map<String, String> properties =
+ ImmutableMap.of(HTTPClient.REST_SOCKET_TIMEOUT_MS, String.valueOf(socketTimeoutMs));
+ String path = "socket/timeout/path";
+
+ try (HTTPClient client = HTTPClient.builder(properties).uri(URI).build()) {
+ HttpRequest mockRequest =
+ request()
+ .withPath("/" + path)
+ .withMethod(HttpMethod.HEAD.name().toUpperCase(Locale.ROOT));
+ // Setting a response delay of 5 seconds to simulate hitting the configured socket timeout of
+ // 2 seconds
+ HttpResponse mockResponse =
+ response()
+ .withStatusCode(200)
+ .withBody("Delayed response")
+ .withDelay(TimeUnit.MILLISECONDS, 5000);
+ mockServer.when(mockRequest).respond(mockResponse);
+
+ Assertions.assertThatThrownBy(() -> client.head(path, ImmutableMap.of(), (unused) -> {}))
+ .cause()
+ .isInstanceOf(SocketTimeoutException.class);
+ }
+ }
+
+ @Test
+ public void testInvalidConnectionTimeout() {
+ testInvalidTimeouts(HTTPClient.REST_CONNECTION_TIMEOUT_MS);
+ }
+
+ @Test
+ public void testInvalidSocketTimeout() {
+ testInvalidTimeouts(HTTPClient.REST_SOCKET_TIMEOUT_MS);
+ }
+
+ private void testInvalidTimeouts(String timeoutMsType) {
Review Comment:
```suggestion
private void checkInvalidTimeout(String timeoutMsType) {
```
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Introduce two properties for reading the connection timeout and socke… [iceberg]
Posted by "harishch1998 (via GitHub)" <gi...@apache.org>.
harishch1998 commented on code in PR #10053:
URL: https://github.com/apache/iceberg/pull/10053#discussion_r1555456733
##########
core/src/test/java/org/apache/iceberg/rest/TestHTTPClient.java:
##########
@@ -133,6 +136,67 @@ public void testDynamicHttpRequestInterceptorLoading() {
assertThat(((TestHttpRequestInterceptor) interceptor).properties).isEqualTo(properties);
}
+ @Test
+ public void testSocketAndConnectionTimeoutSet() {
+ long connectionTimeoutMs = 10L;
+ int socketTimeoutMs = 10;
+ Map<String, String> properties =
+ ImmutableMap.of(
+ HTTPClient.REST_CONNECTION_TIMEOUT_MS, String.valueOf(connectionTimeoutMs),
+ HTTPClient.REST_SOCKET_TIMEOUT_MS, String.valueOf(socketTimeoutMs));
+
+ ConnectionConfig connectionConfig = HTTPClient.configureConnectionConfig(properties);
+ assertThat(connectionConfig).isNotNull();
+ assertThat(connectionConfig.getConnectTimeout().getDuration()).isEqualTo(connectionTimeoutMs);
+ assertThat(connectionConfig.getSocketTimeout().getDuration()).isEqualTo(socketTimeoutMs);
+ }
+
+ @Test
+ public void testSocketTimeout() throws IOException {
+ long socketTimeoutMs = 2000L;
+ Map<String, String> properties =
+ ImmutableMap.of(HTTPClient.REST_SOCKET_TIMEOUT_MS, String.valueOf(socketTimeoutMs));
+ String path = "socket/timeout/path";
+
+ try (HTTPClient client = HTTPClient.builder(properties).uri(URI).build()) {
+ HttpRequest mockRequest =
+ request()
+ .withPath("/" + path)
+ .withMethod(HttpMethod.HEAD.name().toUpperCase(Locale.ROOT));
+ // Setting a response delay of 5 seconds to simulate hitting the configured socket timeout of
+ // 2 seconds
+ HttpResponse mockResponse =
+ response()
+ .withStatusCode(200)
+ .withBody("Delayed response")
+ .withDelay(TimeUnit.MILLISECONDS, 5000);
+ mockServer.when(mockRequest).respond(mockResponse);
+
+ Assertions.assertThatThrownBy(() -> client.head(path, ImmutableMap.of(), (unused) -> {}))
+ .cause()
+ .isInstanceOf(SocketTimeoutException.class);
+ }
+ }
+
+ @Test
+ public void testInvalidConnectionTimeout() {
+ testInvalidTimeouts(HTTPClient.REST_CONNECTION_TIMEOUT_MS);
+ }
+
+ @Test
+ public void testInvalidSocketTimeout() {
+ testInvalidTimeouts(HTTPClient.REST_SOCKET_TIMEOUT_MS);
+ }
+
+ private void testInvalidTimeouts(String timeoutMsType) {
Review Comment:
I'm so sorry for the repeated reviews. Fixed.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Introduce two properties for reading the connection timeout and socke… [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10053:
URL: https://github.com/apache/iceberg/pull/10053#discussion_r1547301639
##########
core/src/test/java/org/apache/iceberg/rest/TestHTTPClient.java:
##########
@@ -133,6 +136,59 @@ public void testDynamicHttpRequestInterceptorLoading() {
assertThat(((TestHttpRequestInterceptor) interceptor).properties).isEqualTo(properties);
}
+ @Test
+ public void testHttpClientGetConnectionConfig() {
+ long connectionTimeoutMs = 10L;
+ int socketTimeoutMs = 10;
+ Map<String, String> properties =
+ ImmutableMap.of(
+ HTTPClient.REST_CONNECTION_TIMEOUT_MS, String.valueOf(connectionTimeoutMs),
+ HTTPClient.REST_SOCKET_TIMEOUT_MS, String.valueOf(socketTimeoutMs));
+
+ ConnectionConfig connectionConfig = HTTPClient.configureConnectionConfig(properties);
+ assertThat(connectionConfig).isNotNull();
+ assertThat(connectionConfig.getConnectTimeout().getDuration()).isEqualTo(connectionTimeoutMs);
+ assertThat(connectionConfig.getSocketTimeout().getDuration()).isEqualTo(socketTimeoutMs);
+ }
+
+ @Test
+ public void testHttpClientWithSocketTimeout() throws IOException {
+ long socketTimeoutMs = 2000L;
+ Map<String, String> properties =
+ ImmutableMap.of(HTTPClient.REST_SOCKET_TIMEOUT_MS, String.valueOf(socketTimeoutMs));
+ String path = "socket/timeout/path";
+
+ try (HTTPClient client = HTTPClient.builder(properties).uri(URI).build()) {
+ HttpRequest mockRequest =
+ request()
+ .withPath("/" + path)
+ .withMethod(HttpMethod.HEAD.name().toUpperCase(Locale.ROOT));
+ // Setting a response delay of 5 seconds to simulate hitting the configured socket timeout of
+ // 2 seconds
+ HttpResponse mockResponse =
+ response()
+ .withStatusCode(200)
+ .withBody("Delayed response")
+ .withDelay(TimeUnit.MILLISECONDS, 5000);
+ mockServer.when(mockRequest).respond(mockResponse);
+
+ Assertions.assertThatThrownBy(() -> client.head(path, ImmutableMap.of(), (unused) -> {}))
+ .cause()
+ .isInstanceOf(SocketTimeoutException.class);
+ }
+ }
+
+ @Test
+ public void testHttpClientInvalidConnectionTimeout() {
+ // We expect a Runtime exception
Review Comment:
this is obvious from the check further above, so no need for the comment
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Introduce two properties for reading the connection timeout and socke… [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10053:
URL: https://github.com/apache/iceberg/pull/10053#discussion_r1547300740
##########
core/src/test/java/org/apache/iceberg/rest/TestHTTPClient.java:
##########
@@ -133,6 +136,59 @@ public void testDynamicHttpRequestInterceptorLoading() {
assertThat(((TestHttpRequestInterceptor) interceptor).properties).isEqualTo(properties);
}
+ @Test
+ public void testHttpClientGetConnectionConfig() {
+ long connectionTimeoutMs = 10L;
+ int socketTimeoutMs = 10;
+ Map<String, String> properties =
+ ImmutableMap.of(
+ HTTPClient.REST_CONNECTION_TIMEOUT_MS, String.valueOf(connectionTimeoutMs),
+ HTTPClient.REST_SOCKET_TIMEOUT_MS, String.valueOf(socketTimeoutMs));
+
+ ConnectionConfig connectionConfig = HTTPClient.configureConnectionConfig(properties);
+ assertThat(connectionConfig).isNotNull();
+ assertThat(connectionConfig.getConnectTimeout().getDuration()).isEqualTo(connectionTimeoutMs);
+ assertThat(connectionConfig.getSocketTimeout().getDuration()).isEqualTo(socketTimeoutMs);
+ }
+
+ @Test
+ public void testHttpClientWithSocketTimeout() throws IOException {
+ long socketTimeoutMs = 2000L;
+ Map<String, String> properties =
+ ImmutableMap.of(HTTPClient.REST_SOCKET_TIMEOUT_MS, String.valueOf(socketTimeoutMs));
+ String path = "socket/timeout/path";
+
+ try (HTTPClient client = HTTPClient.builder(properties).uri(URI).build()) {
+ HttpRequest mockRequest =
+ request()
+ .withPath("/" + path)
+ .withMethod(HttpMethod.HEAD.name().toUpperCase(Locale.ROOT));
+ // Setting a response delay of 5 seconds to simulate hitting the configured socket timeout of
+ // 2 seconds
+ HttpResponse mockResponse =
+ response()
+ .withStatusCode(200)
+ .withBody("Delayed response")
+ .withDelay(TimeUnit.MILLISECONDS, 5000);
+ mockServer.when(mockRequest).respond(mockResponse);
+
+ Assertions.assertThatThrownBy(() -> client.head(path, ImmutableMap.of(), (unused) -> {}))
+ .cause()
+ .isInstanceOf(SocketTimeoutException.class);
+ }
+ }
+
+ @Test
+ public void testHttpClientInvalidConnectionTimeout() {
Review Comment:
please also add a separate test method where the socket timeout is invalid
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Introduce two properties for reading the connection timeout and socke… [iceberg]
Posted by "harishch1998 (via GitHub)" <gi...@apache.org>.
harishch1998 commented on code in PR #10053:
URL: https://github.com/apache/iceberg/pull/10053#discussion_r1544095514
##########
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:
Yeah, these are seconds. I'll fix the naming.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Introduce two properties for reading the connection timeout and socke… [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra merged PR #10053:
URL: https://github.com/apache/iceberg/pull/10053
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org