You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2022/09/18 15:26:31 UTC
[GitHub] [iceberg] JonasJ-ap opened a new pull request, #5787: AWS: Add socket connection timeout for Apache Http Builder
JonasJ-ap opened a new pull request, #5787:
URL: https://github.com/apache/iceberg/pull/5787
User can use `apache.socket-timeout-ms` and `apache.connection-timeout-ms` tags to configure the connection and socket timeout for the `ApacheHttpClient`.
--
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
[GitHub] [iceberg] jackye1995 commented on pull request #5787: AWS: Add socket connection timeout for Apache Http Builder
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on PR #5787:
URL: https://github.com/apache/iceberg/pull/5787#issuecomment-1257080478
Thanks for the work @JonasJ-ap and thanks for the review @amogh-jahagirdar !
--
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
[GitHub] [iceberg] jackye1995 commented on a diff in pull request #5787: AWS: Add socket connection timeout for Apache Http Builder
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on code in PR #5787:
URL: https://github.com/apache/iceberg/pull/5787#discussion_r979077080
##########
aws/src/main/java/org/apache/iceberg/aws/AwsProperties.java:
##########
@@ -352,6 +354,28 @@ public class AwsProperties implements Serializable {
public static final String HTTP_CLIENT_TYPE_DEFAULT = HTTP_CLIENT_TYPE_URLCONNECTION;
+ /**
+ * Used to configure the connection timeout in milliseconds for {@link
+ * software.amazon.awssdk.http.apache.ApacheHttpClient.Builder}. This flag only works when {@link
+ * #HTTP_CLIENT_TYPE} is set to {@link #HTTP_CLIENT_TYPE_APACHE}
+ *
+ * <p>For more details, see
+ * https://sdk.amazonaws.com/java/api/latest/software/amazon/awssdk/http/apache/ApacheHttpClient.Builder.html
+ */
+ public static final String APACHE_HTTP_CLIENT_CONNECTION_TIMEOUT_MS =
Review Comment:
sorry I totally forget we us `http-client` as prefix, so this should probably be `http-client.apache.xxxx`
--
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
[GitHub] [iceberg] jackye1995 commented on a diff in pull request #5787: AWS: Add socket connection timeout for Apache Http Builder
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on code in PR #5787:
URL: https://github.com/apache/iceberg/pull/5787#discussion_r979080448
##########
core/src/main/java/org/apache/iceberg/util/PropertyUtil.java:
##########
@@ -66,6 +66,15 @@ public static long propertyAsLong(
return defaultValue;
}
+ public static Long propertyAsLong(
Review Comment:
looks like the only use case is when the default is null? I think we can name this something as `propertyAsNullableLong` and remove the default value parameter?
--
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
[GitHub] [iceberg] JonasJ-ap commented on a diff in pull request #5787: AWS: Add socket connection timeout for Apache Http Builder
Posted by GitBox <gi...@apache.org>.
JonasJ-ap commented on code in PR #5787:
URL: https://github.com/apache/iceberg/pull/5787#discussion_r979262500
##########
aws/src/main/java/org/apache/iceberg/aws/AwsProperties.java:
##########
@@ -442,6 +466,8 @@ public class AwsProperties implements Serializable {
public static final String LAKE_FORMATION_DB_NAME = "lakeformation.db-name";
private String httpClientType;
+ private Long apacheHttpClientConnectionTimeout;
+ private Long apacheHttpClientSocketTimeout;
Review Comment:
Thank you for your suggestion. I've updated them to `apacheHttpClientConnectionTimeoutMs` and `apacheHttpClientSocketTimeoutMs`
--
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
[GitHub] [iceberg] JonasJ-ap commented on a diff in pull request #5787: AWS: Add socket connection timeout for Apache Http Builder
Posted by GitBox <gi...@apache.org>.
JonasJ-ap commented on code in PR #5787:
URL: https://github.com/apache/iceberg/pull/5787#discussion_r979262351
##########
aws/src/main/java/org/apache/iceberg/aws/AwsProperties.java:
##########
@@ -352,6 +354,28 @@ public class AwsProperties implements Serializable {
public static final String HTTP_CLIENT_TYPE_DEFAULT = HTTP_CLIENT_TYPE_URLCONNECTION;
+ /**
+ * Used to configure the connection timeout in milliseconds for {@link
+ * software.amazon.awssdk.http.apache.ApacheHttpClient.Builder}. This flag only works when {@link
+ * #HTTP_CLIENT_TYPE} is set to {@link #HTTP_CLIENT_TYPE_APACHE}
+ *
+ * <p>For more details, see
+ * https://sdk.amazonaws.com/java/api/latest/software/amazon/awssdk/http/apache/ApacheHttpClient.Builder.html
+ */
+ public static final String APACHE_HTTP_CLIENT_CONNECTION_TIMEOUT_MS =
+ "http-client.apache.connection-timeout-ms";
+
+ /**
+ * Used to configure the socket timeout in milliseconds for {@link
+ * software.amazon.awssdk.http.apache.ApacheHttpClient.Builder}. This flag only works when {@link
+ * #HTTP_CLIENT_TYPE} is set to {@link #HTTP_CLIENT_TYPE_APACHE}
+ *
+ * <p>For more details, see
+ * https://sdk.amazonaws.com/java/api/latest/software/amazon/awssdk/http/apache/ApacheHttpClient.Builder.html
+ */
+ public static final String APACHE_HTTP_CLIENT_SOCKET_TIMEOUT_MS =
Review Comment:
Thank you for your suggestion. I've updated them to `HTTP_CLIENT_APACHE_SOCKET_TIMEOUT_MS` and `HTTP_CLIENT_APACHE_CONNECTION_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
[GitHub] [iceberg] JonasJ-ap commented on a diff in pull request #5787: AWS: Add socket connection timeout for Apache Http Builder
Posted by GitBox <gi...@apache.org>.
JonasJ-ap commented on code in PR #5787:
URL: https://github.com/apache/iceberg/pull/5787#discussion_r979163966
##########
aws/src/main/java/org/apache/iceberg/aws/AwsProperties.java:
##########
@@ -975,4 +1008,19 @@ private <T extends SdkClientBuilder> void configureEndpoint(T builder, String en
builder.endpointOverride(URI.create(endpoint));
}
}
+
+ @VisibleForTesting
+ <T extends ApacheHttpClient.Builder> void configureApacheHttpClientBuilder(T builder) {
+ boolean setConnectionTimeout = apacheHttpClientConnectionTimeout != null;
+ boolean setSocketTimeout = apacheHttpClientSocketTimeout != null;
+ if (setConnectionTimeout && setSocketTimeout) {
+ builder
Review Comment:
Thank you for your review. However, I think I may not get what you mean. Do you refer to line 1017? This method `configureApacheHttpClientBuilder` does not have return value given its return type is `void`. The `builder` standing alone in line 1017 is just due to the autoformatting. Indeed, I design this method so that it can be used like:
```
ApacheHttpClient.builder().applyMutation(this::configureApacheHttpClientBuilder)
```
like the one in line 923.
If I misunderstood your point, please let me know. Thank you again for your help!
--
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
[GitHub] [iceberg] jackye1995 commented on pull request #5787: AWS: Add socket connection timeout for Apache Http Builder
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on PR #5787:
URL: https://github.com/apache/iceberg/pull/5787#issuecomment-1256874503
cc @amogh-jahagirdar @rajarshisarkar @singhpk234 @xiaoxuandev @xingfanx for review
--
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
[GitHub] [iceberg] jackye1995 commented on a diff in pull request #5787: AWS: Add socket connection timeout for Apache Http Builder
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on code in PR #5787:
URL: https://github.com/apache/iceberg/pull/5787#discussion_r977775281
##########
aws/src/main/java/org/apache/iceberg/aws/AwsProperties.java:
##########
@@ -352,6 +354,33 @@ public class AwsProperties implements Serializable {
public static final String HTTP_CLIENT_TYPE_DEFAULT = HTTP_CLIENT_TYPE_URLCONNECTION;
+ /**
+ * Used to configure the connection timeout in milliseconds for {@link
+ * software.amazon.awssdk.http.apache.ApacheHttpClient.Builder}. This flag only works when {@link
+ * #HTTP_CLIENT_TYPE} is set to {@link #HTTP_CLIENT_TYPE_APACHE}
+ *
+ * <p>For more details, see
+ * https://sdk.amazonaws.com/java/api/latest/software/amazon/awssdk/http/apache/ApacheHttpClient.Builder.html
+ */
+ public static final String APACHE_HTTP_CLIENT_CONNECTION_TIMEOUT_MS =
+ "apache.connection-timeout-ms";
+
+ public static final long APACHE_HTTP_CLIENT_CONNECTION_TIMEOUT_MS_DEFAULT =
Review Comment:
I think we should not set a default here, when not set we should not set the value in builder so it takes the default there
--
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
[GitHub] [iceberg] JonasJ-ap commented on a diff in pull request #5787: AWS: Add socket connection timeout for Apache Http Builder
Posted by GitBox <gi...@apache.org>.
JonasJ-ap commented on code in PR #5787:
URL: https://github.com/apache/iceberg/pull/5787#discussion_r979163966
##########
aws/src/main/java/org/apache/iceberg/aws/AwsProperties.java:
##########
@@ -975,4 +1008,19 @@ private <T extends SdkClientBuilder> void configureEndpoint(T builder, String en
builder.endpointOverride(URI.create(endpoint));
}
}
+
+ @VisibleForTesting
+ <T extends ApacheHttpClient.Builder> void configureApacheHttpClientBuilder(T builder) {
+ boolean setConnectionTimeout = apacheHttpClientConnectionTimeout != null;
+ boolean setSocketTimeout = apacheHttpClientSocketTimeout != null;
+ if (setConnectionTimeout && setSocketTimeout) {
+ builder
Review Comment:
Thank you for your review. However, I may not get what you mean. Do you refer to line 1017? This method `configureApacheHttpClientBuilder` does not have any return statement given its return type is `void`. The `builder` standing alone in line 1017 is just due to the autoformatting. Indeed, I design this method so that it can be used like:
```
ApacheHttpClient.builder().applyMutation(this::configureApacheHttpClientBuilder)
```
like the one in line 923.
If I misunderstood your point, please let me know. Thank you again for your help!
--
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
[GitHub] [iceberg] amogh-jahagirdar commented on a diff in pull request #5787: AWS: Add socket connection timeout for Apache Http Builder
Posted by GitBox <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #5787:
URL: https://github.com/apache/iceberg/pull/5787#discussion_r979170137
##########
aws/src/main/java/org/apache/iceberg/aws/AwsProperties.java:
##########
@@ -442,6 +466,8 @@ public class AwsProperties implements Serializable {
public static final String LAKE_FORMATION_DB_NAME = "lakeformation.db-name";
private String httpClientType;
+ private Long apacheHttpClientConnectionTimeout;
+ private Long apacheHttpClientSocketTimeout;
Review Comment:
Let's include the "ms" in the variable name
--
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
[GitHub] [iceberg] JonasJ-ap commented on a diff in pull request #5787: AWS: Add socket connection timeout for Apache Http Builder
Posted by GitBox <gi...@apache.org>.
JonasJ-ap commented on code in PR #5787:
URL: https://github.com/apache/iceberg/pull/5787#discussion_r979262500
##########
aws/src/main/java/org/apache/iceberg/aws/AwsProperties.java:
##########
@@ -442,6 +466,8 @@ public class AwsProperties implements Serializable {
public static final String LAKE_FORMATION_DB_NAME = "lakeformation.db-name";
private String httpClientType;
+ private Long apacheHttpClientConnectionTimeout;
+ private Long apacheHttpClientSocketTimeout;
Review Comment:
Thank you for your suggestion. I've updated them to `httpClientApacheConnectionTimeoutMs` and `httpClientApacheSocketTimeoutMs`
--
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
[GitHub] [iceberg] JonasJ-ap commented on a diff in pull request #5787: AWS: Add socket connection timeout for Apache Http Builder
Posted by GitBox <gi...@apache.org>.
JonasJ-ap commented on code in PR #5787:
URL: https://github.com/apache/iceberg/pull/5787#discussion_r979084595
##########
core/src/main/java/org/apache/iceberg/util/PropertyUtil.java:
##########
@@ -66,6 +66,15 @@ public static long propertyAsLong(
return defaultValue;
}
+ public static Long propertyAsLong(
Review Comment:
Thank you for your suggestions. I've updated the util method.
--
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
[GitHub] [iceberg] JonasJ-ap commented on a diff in pull request #5787: AWS: Add socket connection timeout for Apache Http Builder
Posted by GitBox <gi...@apache.org>.
JonasJ-ap commented on code in PR #5787:
URL: https://github.com/apache/iceberg/pull/5787#discussion_r979163966
##########
aws/src/main/java/org/apache/iceberg/aws/AwsProperties.java:
##########
@@ -975,4 +1008,19 @@ private <T extends SdkClientBuilder> void configureEndpoint(T builder, String en
builder.endpointOverride(URI.create(endpoint));
}
}
+
+ @VisibleForTesting
+ <T extends ApacheHttpClient.Builder> void configureApacheHttpClientBuilder(T builder) {
+ boolean setConnectionTimeout = apacheHttpClientConnectionTimeout != null;
+ boolean setSocketTimeout = apacheHttpClientSocketTimeout != null;
+ if (setConnectionTimeout && setSocketTimeout) {
+ builder
Review Comment:
Thank you for your review. I will simplify the mechanism
--
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
[GitHub] [iceberg] amogh-jahagirdar commented on a diff in pull request #5787: AWS: Add socket connection timeout for Apache Http Builder
Posted by GitBox <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #5787:
URL: https://github.com/apache/iceberg/pull/5787#discussion_r979170137
##########
aws/src/main/java/org/apache/iceberg/aws/AwsProperties.java:
##########
@@ -442,6 +466,8 @@ public class AwsProperties implements Serializable {
public static final String LAKE_FORMATION_DB_NAME = "lakeformation.db-name";
private String httpClientType;
+ private Long apacheHttpClientConnectionTimeout;
+ private Long apacheHttpClientSocketTimeout;
Review Comment:
Let's include the "ms" or "millis" (whatever the existing convention is in the codebase) in the variable name
--
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
[GitHub] [iceberg] jackye1995 commented on a diff in pull request #5787: AWS: Add socket connection timeout for Apache Http Builder
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on code in PR #5787:
URL: https://github.com/apache/iceberg/pull/5787#discussion_r977768161
##########
aws/src/main/java/org/apache/iceberg/aws/AwsProperties.java:
##########
@@ -352,6 +354,33 @@ public class AwsProperties implements Serializable {
public static final String HTTP_CLIENT_TYPE_DEFAULT = HTTP_CLIENT_TYPE_URLCONNECTION;
+ /**
+ * Used to configure the connection timeout in milliseconds for {@link
+ * software.amazon.awssdk.http.apache.ApacheHttpClient.Builder}. This flag only works when {@link
+ * #HTTP_CLIENT_TYPE} is set to {@link #HTTP_CLIENT_TYPE_APACHE}
+ *
+ * <p>For more details, see
+ * https://sdk.amazonaws.com/java/api/latest/software/amazon/awssdk/http/apache/ApacheHttpClient.Builder.html
+ */
+ public static final String APACHE_HTTP_CLIENT_CONNECTION_TIMEOUT_MS =
+ "apache.connection-timeout-ms";
Review Comment:
I think this config should be prefixed by `client` indicating this is a config for AWS clients. Similar also for other config names
--
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
[GitHub] [iceberg] jackye1995 merged pull request #5787: AWS: Add socket connection timeout for Apache Http Builder
Posted by GitBox <gi...@apache.org>.
jackye1995 merged PR #5787:
URL: https://github.com/apache/iceberg/pull/5787
--
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
[GitHub] [iceberg] JonasJ-ap commented on a diff in pull request #5787: AWS: Add socket connection timeout for Apache Http Builder
Posted by GitBox <gi...@apache.org>.
JonasJ-ap commented on code in PR #5787:
URL: https://github.com/apache/iceberg/pull/5787#discussion_r979163966
##########
aws/src/main/java/org/apache/iceberg/aws/AwsProperties.java:
##########
@@ -975,4 +1008,19 @@ private <T extends SdkClientBuilder> void configureEndpoint(T builder, String en
builder.endpointOverride(URI.create(endpoint));
}
}
+
+ @VisibleForTesting
+ <T extends ApacheHttpClient.Builder> void configureApacheHttpClientBuilder(T builder) {
+ boolean setConnectionTimeout = apacheHttpClientConnectionTimeout != null;
+ boolean setSocketTimeout = apacheHttpClientSocketTimeout != null;
+ if (setConnectionTimeout && setSocketTimeout) {
+ builder
Review Comment:
Thank you for your review. However, I may not get what you mean. Do you refer to line 1017? This method `configureApacheHttpClientBuilder` does not have return value given its return type is `void`. The `builder` standing alone in line 1017 is just due to the autoformatting. Indeed, I design this method so that it can be used like:
```
ApacheHttpClient.builder().applyMutation(this::configureApacheHttpClientBuilder)
```
like the one in line 923.
If I misunderstood your point, please let me know. Thank you again for your help!
--
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
[GitHub] [iceberg] jackye1995 commented on a diff in pull request #5787: AWS: Add socket connection timeout for Apache Http Builder
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on code in PR #5787:
URL: https://github.com/apache/iceberg/pull/5787#discussion_r979169518
##########
aws/src/main/java/org/apache/iceberg/aws/AwsProperties.java:
##########
@@ -352,6 +354,28 @@ public class AwsProperties implements Serializable {
public static final String HTTP_CLIENT_TYPE_DEFAULT = HTTP_CLIENT_TYPE_URLCONNECTION;
+ /**
+ * Used to configure the connection timeout in milliseconds for {@link
+ * software.amazon.awssdk.http.apache.ApacheHttpClient.Builder}. This flag only works when {@link
+ * #HTTP_CLIENT_TYPE} is set to {@link #HTTP_CLIENT_TYPE_APACHE}
+ *
+ * <p>For more details, see
+ * https://sdk.amazonaws.com/java/api/latest/software/amazon/awssdk/http/apache/ApacheHttpClient.Builder.html
+ */
+ public static final String APACHE_HTTP_CLIENT_CONNECTION_TIMEOUT_MS =
+ "http-client.apache.connection-timeout-ms";
+
+ /**
+ * Used to configure the socket timeout in milliseconds for {@link
+ * software.amazon.awssdk.http.apache.ApacheHttpClient.Builder}. This flag only works when {@link
+ * #HTTP_CLIENT_TYPE} is set to {@link #HTTP_CLIENT_TYPE_APACHE}
+ *
+ * <p>For more details, see
+ * https://sdk.amazonaws.com/java/api/latest/software/amazon/awssdk/http/apache/ApacheHttpClient.Builder.html
+ */
+ public static final String APACHE_HTTP_CLIENT_SOCKET_TIMEOUT_MS =
Review Comment:
can we match variable name with config name? e.g. `HTTP_CLIENT_APACHE_SOCKET_TIMEOUT_MS`, `httpClientApacheSocketTimeout`
--
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
[GitHub] [iceberg] jackye1995 commented on a diff in pull request #5787: AWS: Add socket connection timeout for Apache Http Builder
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on code in PR #5787:
URL: https://github.com/apache/iceberg/pull/5787#discussion_r979088849
##########
aws/src/main/java/org/apache/iceberg/aws/AwsProperties.java:
##########
@@ -975,4 +1008,19 @@ private <T extends SdkClientBuilder> void configureEndpoint(T builder, String en
builder.endpointOverride(URI.create(endpoint));
}
}
+
+ @VisibleForTesting
+ <T extends ApacheHttpClient.Builder> void configureApacheHttpClientBuilder(T builder) {
+ boolean setConnectionTimeout = apacheHttpClientConnectionTimeout != null;
+ boolean setSocketTimeout = apacheHttpClientSocketTimeout != null;
+ if (setConnectionTimeout && setSocketTimeout) {
+ builder
Review Comment:
we don't need to do this, when you do `builder.xxxx` it automatically mutates the builder itself. Returning itself is just to facilitate method chaining.
--
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
[GitHub] [iceberg] jackye1995 commented on a diff in pull request #5787: AWS: Add socket connection timeout for Apache Http Builder
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on code in PR #5787:
URL: https://github.com/apache/iceberg/pull/5787#discussion_r977775281
##########
aws/src/main/java/org/apache/iceberg/aws/AwsProperties.java:
##########
@@ -352,6 +354,33 @@ public class AwsProperties implements Serializable {
public static final String HTTP_CLIENT_TYPE_DEFAULT = HTTP_CLIENT_TYPE_URLCONNECTION;
+ /**
+ * Used to configure the connection timeout in milliseconds for {@link
+ * software.amazon.awssdk.http.apache.ApacheHttpClient.Builder}. This flag only works when {@link
+ * #HTTP_CLIENT_TYPE} is set to {@link #HTTP_CLIENT_TYPE_APACHE}
+ *
+ * <p>For more details, see
+ * https://sdk.amazonaws.com/java/api/latest/software/amazon/awssdk/http/apache/ApacheHttpClient.Builder.html
+ */
+ public static final String APACHE_HTTP_CLIENT_CONNECTION_TIMEOUT_MS =
+ "apache.connection-timeout-ms";
+
+ public static final long APACHE_HTTP_CLIENT_CONNECTION_TIMEOUT_MS_DEFAULT =
Review Comment:
we should not set a default here, when not set we should not set the value in builder so it takes the default there
--
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
[GitHub] [iceberg] JonasJ-ap commented on pull request #5787: AWS: Add socket connection timeout for Apache Http Builder
Posted by GitBox <gi...@apache.org>.
JonasJ-ap commented on PR #5787:
URL: https://github.com/apache/iceberg/pull/5787#issuecomment-1257072452
Rebase to master branch
--
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
[GitHub] [iceberg] JonasJ-ap commented on a diff in pull request #5787: AWS: Add socket connection timeout for Apache Http Builder
Posted by GitBox <gi...@apache.org>.
JonasJ-ap commented on code in PR #5787:
URL: https://github.com/apache/iceberg/pull/5787#discussion_r979163966
##########
aws/src/main/java/org/apache/iceberg/aws/AwsProperties.java:
##########
@@ -975,4 +1008,19 @@ private <T extends SdkClientBuilder> void configureEndpoint(T builder, String en
builder.endpointOverride(URI.create(endpoint));
}
}
+
+ @VisibleForTesting
+ <T extends ApacheHttpClient.Builder> void configureApacheHttpClientBuilder(T builder) {
+ boolean setConnectionTimeout = apacheHttpClientConnectionTimeout != null;
+ boolean setSocketTimeout = apacheHttpClientSocketTimeout != null;
+ if (setConnectionTimeout && setSocketTimeout) {
+ builder
Review Comment:
Thank you for your suggestions. I will simplify the mechanism
--
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
[GitHub] [iceberg] JonasJ-ap commented on a diff in pull request #5787: AWS: Add socket connection timeout for Apache Http Builder
Posted by GitBox <gi...@apache.org>.
JonasJ-ap commented on code in PR #5787:
URL: https://github.com/apache/iceberg/pull/5787#discussion_r979074174
##########
aws/src/test/java/org/apache/iceberg/aws/TestAwsProperties.java:
##########
@@ -204,4 +209,139 @@ public void testS3FileIoSessionCredentialsConfiguration() {
"secret",
capturedAwsCredentialsProvider.resolveCredentials().secretAccessKey());
}
+
+ @Test
+ public void testUrlHttpClientConfiguration() {
+ Map<String, String> properties = Maps.newHashMap();
+ properties.put(AwsProperties.HTTP_CLIENT_TYPE, "urlconnection");
+ AwsProperties awsProperties = new AwsProperties(properties);
+ S3ClientBuilder mockS3ClientBuilder = Mockito.mock(S3ClientBuilder.class);
+ ArgumentCaptor<SdkHttpClient.Builder> httpClientBuilderCaptor =
+ ArgumentCaptor.forClass(SdkHttpClient.Builder.class);
+
+ awsProperties.applyHttpClientConfigurations(mockS3ClientBuilder);
+ Mockito.verify(mockS3ClientBuilder).httpClientBuilder(httpClientBuilderCaptor.capture());
+ SdkHttpClient.Builder capturedHttpClientBuilder = httpClientBuilderCaptor.getValue();
+
+ Assert.assertTrue(
+ "Should use url connection http client",
+ capturedHttpClientBuilder instanceof UrlConnectionHttpClient.Builder);
+ }
+
+ @Test
+ public void testApacheHttpClientConfiguration() {
+ Map<String, String> properties = Maps.newHashMap();
+ properties.put(AwsProperties.HTTP_CLIENT_TYPE, "apache");
+ AwsProperties awsProperties = new AwsProperties(properties);
+ S3ClientBuilder mockS3ClientBuilder = Mockito.mock(S3ClientBuilder.class);
+ ArgumentCaptor<SdkHttpClient.Builder> httpClientBuilderCaptor =
+ ArgumentCaptor.forClass(SdkHttpClient.Builder.class);
+
+ awsProperties.applyHttpClientConfigurations(mockS3ClientBuilder);
+ Mockito.verify(mockS3ClientBuilder).httpClientBuilder(httpClientBuilderCaptor.capture());
+ SdkHttpClient.Builder capturedHttpClientBuilder = httpClientBuilderCaptor.getValue();
+ Assert.assertTrue(
+ "Should use apache http client",
+ capturedHttpClientBuilder instanceof ApacheHttpClient.Builder);
+ }
+
+ @Test
+ public void testInvalidHttpClientType() {
+ Map<String, String> properties = Maps.newHashMap();
+ properties.put(AwsProperties.HTTP_CLIENT_TYPE, "test");
+ AwsProperties awsProperties = new AwsProperties(properties);
+ S3ClientBuilder s3ClientBuilder = S3Client.builder();
+
+ AssertHelpers.assertThrows(
+ "should not support http client types other than urlconnection and apache",
+ IllegalArgumentException.class,
+ "Unrecognized HTTP client type",
+ () -> awsProperties.applyHttpClientConfigurations(s3ClientBuilder));
+ }
+
+ @Test
+ public void testApacheConnectionSocketTimeoutConfiguration() {
Review Comment:
These are newly added unit tests for the timeout configurations
--
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