You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "JonasJ-ap (via GitHub)" <gi...@apache.org> on 2023/02/04 23:46:03 UTC
[GitHub] [iceberg] JonasJ-ap opened a new pull request, #6746: AWS: Load HttpClientBuilder dynamically to avoid runtime deps of both urlconnection and apache client
JonasJ-ap opened a new pull request, #6746:
URL: https://github.com/apache/iceberg/pull/6746
## Problem Addressed
This PR fix the problem described in issue #6715 by using reflection to instantiate the httpclient configuration impl class to avoid runtime deps of both `url-connection-client` and `apache-httpclient`
## Test Environment
Spark3.3, Scala 2.12.
scripts to spawn spark shell:
```bash
BRANCH_NAME=bug_fix_aws_httpclient_conflict_prefix_map
DEPENDENCIES=""
# add AWS dependnecy
AWS_SDK_VERSION=2.17.257
AWS_MAVEN_GROUP=software.amazon.awssdk
AWS_PACKAGES=(
"apache-client"
"s3"
"glue"
"kms"
"iam"
"sts"
"dynamodb"
)
for pkg in "${AWS_PACKAGES[@]}"; do
DEPENDENCIES+="$AWS_MAVEN_GROUP:$pkg:$AWS_SDK_VERSION,"
done
JARS="iceberg-spark-runtime-3.3_$BRANCH_NAME.jar"
# start Spark SQL client shell
spark-shell --packages=$DEPENDENCIES --jars=$JARS\
--conf spark.sql.extensions=org.apache.iceberg.spark.extensions.IcebergSparkSessionExtensions \
--conf spark.sql.catalog.demo=org.apache.iceberg.spark.SparkCatalog \
--conf spark.sql.catalog.demo.catalog-impl=org.apache.iceberg.aws.glue.GlueCatalog \
--conf spark.sql.catalog.demo.io-impl=org.apache.iceberg.aws.s3.S3FileIO \
--conf spark.sql.catalog.demo.warehouse=s3://gluetestjonas/warehouse \
--conf spark.sql.catalog.demo.http-client.type=apache
```
### Test commands:
```bash
scala> val data = spark.range(0, 10)
scala> data.writeTo("demo.default.test1").create()
```
### Before the fix:
will raise `java.lang.NoClassDefFoundError`
```bash
java.lang.NoClassDefFoundError: software/amazon/awssdk/http/urlconnection/UrlConnectionHttpClient$Builder
at java.base/java.lang.Class.getDeclaredMethods0(Native Method)
at java.base/java.lang.Class.privateGetDeclaredMethods(Class.java:3458)
at java.base/java.lang.Class.getDeclaredMethod(Class.java:2726)
at java.base/java.io.ObjectStreamClass.getPrivateMethod(ObjectStreamClass.java:1525)
at java.base/java.io.ObjectStreamClass$2.run(ObjectStreamClass.java:413)
at java.base/java.io.ObjectStreamClass$2.run(ObjectStreamClass.java:384)
at java.base/java.security.AccessController.doPrivileged(AccessController.java:318)
at java.base/java.io.ObjectStreamClass.<init>(ObjectStreamClass.java:384)
at java.base/java.io.ObjectStreamClass$Caches$1.computeValue(ObjectStreamClass.java:110)
at java.base/java.io.ObjectStreamClass$Caches$1.computeValue(ObjectStreamClass.java:107)
at java.base/java.io.ClassCache$1.computeValue(ClassCache.java:73)
at java.base/java.io.ClassCache$1.computeValue(ClassCache.java:70)
at java.base/java.lang.ClassValue.getFromHashMap(ClassValue.java:229)
at java.base/java.lang.ClassValue.getFromBackup(ClassValue.java:211)
at java.base/java.lang.ClassValue.get(ClassValue.java:117)
at java.base/java.io.ClassCache.get(ClassCache.java:84)
at java.base/java.io.ObjectStreamClass.lookup(ObjectStreamClass.java:363)
at
```
### After the fix
Successfully create the table
--
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 #6746: AWS: Load HttpClientBuilder dynamically to avoid runtime deps of both urlconnection and apache client
Posted by "JonasJ-ap (via GitHub)" <gi...@apache.org>.
JonasJ-ap commented on code in PR #6746:
URL: https://github.com/apache/iceberg/pull/6746#discussion_r1096958822
##########
aws/src/main/java/org/apache/iceberg/aws/AwsProperties.java:
##########
@@ -1314,55 +1294,27 @@ private <T extends SdkClientBuilder> void configureEndpoint(T builder, String en
}
}
- @VisibleForTesting
- <T extends UrlConnectionHttpClient.Builder> void configureUrlConnectionHttpClientBuilder(
- T builder) {
- if (httpClientUrlConnectionConnectionTimeoutMs != null) {
- builder.connectionTimeout(Duration.ofMillis(httpClientUrlConnectionConnectionTimeoutMs));
- }
-
- if (httpClientUrlConnectionSocketTimeoutMs != null) {
- builder.socketTimeout(Duration.ofMillis(httpClientUrlConnectionSocketTimeoutMs));
- }
- }
-
- @VisibleForTesting
- <T extends ApacheHttpClient.Builder> void configureApacheHttpClientBuilder(T builder) {
- if (httpClientApacheConnectionTimeoutMs != null) {
- builder.connectionTimeout(Duration.ofMillis(httpClientApacheConnectionTimeoutMs));
- }
-
- if (httpClientApacheSocketTimeoutMs != null) {
- builder.socketTimeout(Duration.ofMillis(httpClientApacheSocketTimeoutMs));
- }
-
- if (httpClientApacheConnectionAcquisitionTimeoutMs != null) {
- builder.connectionAcquisitionTimeout(
- Duration.ofMillis(httpClientApacheConnectionAcquisitionTimeoutMs));
- }
-
- if (httpClientApacheConnectionMaxIdleTimeMs != null) {
- builder.connectionMaxIdleTime(Duration.ofMillis(httpClientApacheConnectionMaxIdleTimeMs));
- }
-
- if (httpClientApacheConnectionTimeToLiveMs != null) {
- builder.connectionTimeToLive(Duration.ofMillis(httpClientApacheConnectionTimeToLiveMs));
- }
-
- if (httpClientApacheExpectContinueEnabled != null) {
- builder.expectContinueEnabled(httpClientApacheExpectContinueEnabled);
- }
-
- if (httpClientApacheMaxConnections != null) {
- builder.maxConnections(httpClientApacheMaxConnections);
- }
-
- if (httpClientApacheTcpKeepAliveEnabled != null) {
- builder.tcpKeepAlive(httpClientApacheTcpKeepAliveEnabled);
- }
-
- if (httpClientApacheUseIdleConnectionReaperEnabled != null) {
- builder.useIdleConnectionReaper(httpClientApacheUseIdleConnectionReaperEnabled);
+ private HttpClientConfigurations loadHttpClientConfigurations(
+ String impl, Map<String, String> httpClientProperties) {
+ DynConstructors.Ctor<HttpClientConfigurations> ctor;
+ try {
+ ctor =
+ DynConstructors.builder(HttpClientConfigurations.class).hiddenImpl(impl).buildChecked();
Review Comment:
Thank you for your suggestions. Currently I switch the implementation to define `HttpClientConfigurations`.
For the `DynMethods` way, it seems the final code will be something like:
```java
case HTTP_CLIENT_TYPE_URLCONNECTION:
Object httpClientConfigurations = loadHttpClientConfigurations(
UrlConnectionHttpClientConfigurations.class.getName(),
urlConnectionHttpClientProperties);
((UrlConnectionHttpClientConfigurations)httpClientConfigurations).configureHttpClientBuilder(builder);
```
since we no longer have a interface or superclass. I personally feel it wierd to return and then cast an `Object` directly here. So I vote +1 for the having a package-private abstract class.
Also, now I am curious about why the previous impl
```
interface HttpClientConfigurations {
...
```
does not work. It seems having a package-private interface also will be similar to a package-private abstract class in this case. Is there any design purpose to not use package-private interface or do I misunderstand something here. Thank you in advance 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] JonasJ-ap commented on pull request #6746: AWS: Load HttpClientBuilder dynamically to avoid runtime deps of both urlconnection and apache client
Posted by "JonasJ-ap (via GitHub)" <gi...@apache.org>.
JonasJ-ap commented on PR #6746:
URL: https://github.com/apache/iceberg/pull/6746#issuecomment-1423889742
Sorry for the late update. It took me sometime to construct the EKS environment properly.
## Test Environment
AWS EKS: 1.24, Spark 3.1.2
## Test spark job / k8s job config:
```java
import org.apache.spark.sql.SparkSession
object GlueApp {
def main(sysArgs: Array[String]) {
val spark: SparkSession = SparkSession.builder.
config("spark.master", "local").
config("spark.sql.extensions", "org.apache.iceberg.spark.extensions.IcebergSparkSessionExtensions").
config("spark.sql.catalog.demo", "org.apache.iceberg.spark.SparkCatalog").
config("spark.sql.catalog.demo.io-impl", "org.apache.iceberg.aws.s3.S3FileIO").
config("spark.sql.catalog.demo.warehouse", "s3a://gluetestjonas/warehouse").
config("spark.sql.catalog.demo.catalog-impl", "org.apache.iceberg.aws.glue.GlueCatalog").
config("spark.sql.catalog.demo.http-client.type", "apache").
getOrCreate()
val data = spark.range(0, 100)
data.writeTo("demo.default.table100").createOrReplace()
spark.sql("SELECT * FROM demo.default.table100").show()
// read using SQL
// spark.sql("SELECT * FROM demo.reviews.book_reviews").show()
}
}
```
```bash
name: spark
image: jonasjiang/spark-eks:v3.1.2
args: [
"/bin/sh",
"-c",
"/opt/spark/bin/spark-submit \
--master k8s://https://kubernetes.default.svc.cluster.local:443 \
--deploy-mode cluster \
--name spark-eks \
--class GlueApp \
--jars s3a://gluetestjonas/jars/iceberg-spark-runtime-3.1_bug_fix_aws_httpclient_conflict_prefix_map.jar \
--packages software.amazon.awssdk:apache-client:2.17.257,software.amazon.awssdk:s3:2.17.257,software.amazon.awssdk:glue:2.17.257,software.amazon.awssdk:kms:2.17.257,software.amazon.awssdk:iam:2.17.257,software.amazon.awssdk:sts:2.17.257,software.amazon.awssdk:dynamodb:2.17.257 \
...
--conf spark.kubernetes.container.image=jonasjiang/spark-eks:v3.1.2 \
...
--conf spark.kubernetes.authenticate.driver.serviceAccountName=spark \
--conf spark.hadoop.fs.s3a.aws.credentials.provider=com.amazonaws.auth.WebIdentityTokenCredentialsProvider \
--conf spark.kubernetes.authenticate.submission.caCertFile=/var/run/secrets/kubernetes.io/serviceaccount/ca.crt \
--conf spark.kubernetes.authenticate.submission.oauthTokenFile=/var/run/secrets/kubernetes.io/serviceaccount/token \
s3a://gluetestjonas/jars/scalar-glue_apache.jar
```
### Test result:
Current master branch had the following error:
This PR was fine:
Indicating that we no longer need `urlconnection` when set http-client.type to `apache`
--
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 #6746: AWS: Load HttpClientBuilder dynamically to avoid runtime deps of both urlconnection and apache client
Posted by "jackye1995 (via GitHub)" <gi...@apache.org>.
jackye1995 commented on code in PR #6746:
URL: https://github.com/apache/iceberg/pull/6746#discussion_r1096608183
##########
aws/src/main/java/org/apache/iceberg/aws/AwsProperties.java:
##########
@@ -1314,55 +1294,27 @@ private <T extends SdkClientBuilder> void configureEndpoint(T builder, String en
}
}
- @VisibleForTesting
- <T extends UrlConnectionHttpClient.Builder> void configureUrlConnectionHttpClientBuilder(
- T builder) {
- if (httpClientUrlConnectionConnectionTimeoutMs != null) {
- builder.connectionTimeout(Duration.ofMillis(httpClientUrlConnectionConnectionTimeoutMs));
- }
-
- if (httpClientUrlConnectionSocketTimeoutMs != null) {
- builder.socketTimeout(Duration.ofMillis(httpClientUrlConnectionSocketTimeoutMs));
- }
- }
-
- @VisibleForTesting
- <T extends ApacheHttpClient.Builder> void configureApacheHttpClientBuilder(T builder) {
- if (httpClientApacheConnectionTimeoutMs != null) {
- builder.connectionTimeout(Duration.ofMillis(httpClientApacheConnectionTimeoutMs));
- }
-
- if (httpClientApacheSocketTimeoutMs != null) {
- builder.socketTimeout(Duration.ofMillis(httpClientApacheSocketTimeoutMs));
- }
-
- if (httpClientApacheConnectionAcquisitionTimeoutMs != null) {
- builder.connectionAcquisitionTimeout(
- Duration.ofMillis(httpClientApacheConnectionAcquisitionTimeoutMs));
- }
-
- if (httpClientApacheConnectionMaxIdleTimeMs != null) {
- builder.connectionMaxIdleTime(Duration.ofMillis(httpClientApacheConnectionMaxIdleTimeMs));
- }
-
- if (httpClientApacheConnectionTimeToLiveMs != null) {
- builder.connectionTimeToLive(Duration.ofMillis(httpClientApacheConnectionTimeToLiveMs));
- }
-
- if (httpClientApacheExpectContinueEnabled != null) {
- builder.expectContinueEnabled(httpClientApacheExpectContinueEnabled);
- }
-
- if (httpClientApacheMaxConnections != null) {
- builder.maxConnections(httpClientApacheMaxConnections);
- }
-
- if (httpClientApacheTcpKeepAliveEnabled != null) {
- builder.tcpKeepAlive(httpClientApacheTcpKeepAliveEnabled);
- }
-
- if (httpClientApacheUseIdleConnectionReaperEnabled != null) {
- builder.useIdleConnectionReaper(httpClientApacheUseIdleConnectionReaperEnabled);
+ private HttpClientConfigurations loadHttpClientConfigurations(
+ String impl, Map<String, String> httpClientProperties) {
+ DynConstructors.Ctor<HttpClientConfigurations> ctor;
+ try {
+ ctor =
+ DynConstructors.builder(HttpClientConfigurations.class).hiddenImpl(impl).buildChecked();
Review Comment:
trying to see if we can avoid having to create a new public interface like `HttpClientConfigurations`. We can have some protected static methods, and dynamically load them using something like
```
DynMethods.builder("initialize")
.impl(impl, Map.class)
.buildStaticChecked()
```
--
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] stevenzwu commented on a diff in pull request #6746: AWS: Load HttpClientBuilder dynamically to avoid runtime deps of both urlconnection and apache client
Posted by "stevenzwu (via GitHub)" <gi...@apache.org>.
stevenzwu commented on code in PR #6746:
URL: https://github.com/apache/iceberg/pull/6746#discussion_r1099444467
##########
aws/src/main/java/org/apache/iceberg/aws/AwsProperties.java:
##########
@@ -1314,55 +1270,18 @@ private <T extends SdkClientBuilder> void configureEndpoint(T builder, String en
}
}
- @VisibleForTesting
- <T extends UrlConnectionHttpClient.Builder> void configureUrlConnectionHttpClientBuilder(
- T builder) {
- if (httpClientUrlConnectionConnectionTimeoutMs != null) {
- builder.connectionTimeout(Duration.ofMillis(httpClientUrlConnectionConnectionTimeoutMs));
- }
-
- if (httpClientUrlConnectionSocketTimeoutMs != null) {
- builder.socketTimeout(Duration.ofMillis(httpClientUrlConnectionSocketTimeoutMs));
- }
- }
-
- @VisibleForTesting
- <T extends ApacheHttpClient.Builder> void configureApacheHttpClientBuilder(T builder) {
- if (httpClientApacheConnectionTimeoutMs != null) {
- builder.connectionTimeout(Duration.ofMillis(httpClientApacheConnectionTimeoutMs));
- }
-
- if (httpClientApacheSocketTimeoutMs != null) {
- builder.socketTimeout(Duration.ofMillis(httpClientApacheSocketTimeoutMs));
- }
-
- if (httpClientApacheConnectionAcquisitionTimeoutMs != null) {
- builder.connectionAcquisitionTimeout(
- Duration.ofMillis(httpClientApacheConnectionAcquisitionTimeoutMs));
- }
-
- if (httpClientApacheConnectionMaxIdleTimeMs != null) {
- builder.connectionMaxIdleTime(Duration.ofMillis(httpClientApacheConnectionMaxIdleTimeMs));
- }
-
- if (httpClientApacheConnectionTimeToLiveMs != null) {
- builder.connectionTimeToLive(Duration.ofMillis(httpClientApacheConnectionTimeToLiveMs));
- }
-
- if (httpClientApacheExpectContinueEnabled != null) {
- builder.expectContinueEnabled(httpClientApacheExpectContinueEnabled);
- }
-
- if (httpClientApacheMaxConnections != null) {
- builder.maxConnections(httpClientApacheMaxConnections);
- }
-
- if (httpClientApacheTcpKeepAliveEnabled != null) {
- builder.tcpKeepAlive(httpClientApacheTcpKeepAliveEnabled);
- }
-
- if (httpClientApacheUseIdleConnectionReaperEnabled != null) {
- builder.useIdleConnectionReaper(httpClientApacheUseIdleConnectionReaperEnabled);
+ private Object loadHttpClientConfigurations(String impl) {
Review Comment:
nit: maybe add a brief Javadoc to explain why we are doing the reflection here and link to the github issue for more context.
--
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] stevenzwu commented on a diff in pull request #6746: AWS: Load HttpClientBuilder dynamically to avoid runtime deps of both urlconnection and apache client
Posted by "stevenzwu (via GitHub)" <gi...@apache.org>.
stevenzwu commented on code in PR #6746:
URL: https://github.com/apache/iceberg/pull/6746#discussion_r1096946728
##########
aws/src/main/java/org/apache/iceberg/aws/AwsProperties.java:
##########
@@ -1314,55 +1281,27 @@ private <T extends SdkClientBuilder> void configureEndpoint(T builder, String en
}
}
- @VisibleForTesting
- <T extends UrlConnectionHttpClient.Builder> void configureUrlConnectionHttpClientBuilder(
- T builder) {
- if (httpClientUrlConnectionConnectionTimeoutMs != null) {
- builder.connectionTimeout(Duration.ofMillis(httpClientUrlConnectionConnectionTimeoutMs));
- }
-
- if (httpClientUrlConnectionSocketTimeoutMs != null) {
- builder.socketTimeout(Duration.ofMillis(httpClientUrlConnectionSocketTimeoutMs));
- }
- }
-
- @VisibleForTesting
- <T extends ApacheHttpClient.Builder> void configureApacheHttpClientBuilder(T builder) {
- if (httpClientApacheConnectionTimeoutMs != null) {
- builder.connectionTimeout(Duration.ofMillis(httpClientApacheConnectionTimeoutMs));
- }
-
- if (httpClientApacheSocketTimeoutMs != null) {
- builder.socketTimeout(Duration.ofMillis(httpClientApacheSocketTimeoutMs));
- }
-
- if (httpClientApacheConnectionAcquisitionTimeoutMs != null) {
- builder.connectionAcquisitionTimeout(
- Duration.ofMillis(httpClientApacheConnectionAcquisitionTimeoutMs));
- }
-
- if (httpClientApacheConnectionMaxIdleTimeMs != null) {
- builder.connectionMaxIdleTime(Duration.ofMillis(httpClientApacheConnectionMaxIdleTimeMs));
- }
-
- if (httpClientApacheConnectionTimeToLiveMs != null) {
- builder.connectionTimeToLive(Duration.ofMillis(httpClientApacheConnectionTimeToLiveMs));
- }
-
- if (httpClientApacheExpectContinueEnabled != null) {
- builder.expectContinueEnabled(httpClientApacheExpectContinueEnabled);
- }
-
- if (httpClientApacheMaxConnections != null) {
- builder.maxConnections(httpClientApacheMaxConnections);
- }
-
- if (httpClientApacheTcpKeepAliveEnabled != null) {
- builder.tcpKeepAlive(httpClientApacheTcpKeepAliveEnabled);
- }
-
- if (httpClientApacheUseIdleConnectionReaperEnabled != null) {
- builder.useIdleConnectionReaper(httpClientApacheUseIdleConnectionReaperEnabled);
+ private HttpClientConfigurations loadHttpClientConfigurations(
+ String impl, Map<String, String> httpClientProperties) {
+ DynConstructors.Ctor<HttpClientConfigurations> ctor;
+ try {
+ ctor =
+ DynConstructors.builder(HttpClientConfigurations.class).hiddenImpl(impl).buildChecked();
+ HttpClientConfigurations httpClientConfigurations = ctor.newInstance();
+ httpClientConfigurations.initialize(httpClientProperties);
+ return httpClientConfigurations;
+ } catch (NoSuchMethodException e) {
+ throw new IllegalArgumentException(
+ String.format(
+ "Cannot initialize HttpClientConfigurations Implementation %s: %s",
+ impl, e.getMessage()),
Review Comment:
no need to add `e.getMessage` since `e` is already included in the cause.
##########
aws/src/main/java/org/apache/iceberg/aws/AwsProperties.java:
##########
@@ -1314,55 +1281,27 @@ private <T extends SdkClientBuilder> void configureEndpoint(T builder, String en
}
}
- @VisibleForTesting
- <T extends UrlConnectionHttpClient.Builder> void configureUrlConnectionHttpClientBuilder(
- T builder) {
- if (httpClientUrlConnectionConnectionTimeoutMs != null) {
- builder.connectionTimeout(Duration.ofMillis(httpClientUrlConnectionConnectionTimeoutMs));
- }
-
- if (httpClientUrlConnectionSocketTimeoutMs != null) {
- builder.socketTimeout(Duration.ofMillis(httpClientUrlConnectionSocketTimeoutMs));
- }
- }
-
- @VisibleForTesting
- <T extends ApacheHttpClient.Builder> void configureApacheHttpClientBuilder(T builder) {
- if (httpClientApacheConnectionTimeoutMs != null) {
- builder.connectionTimeout(Duration.ofMillis(httpClientApacheConnectionTimeoutMs));
- }
-
- if (httpClientApacheSocketTimeoutMs != null) {
- builder.socketTimeout(Duration.ofMillis(httpClientApacheSocketTimeoutMs));
- }
-
- if (httpClientApacheConnectionAcquisitionTimeoutMs != null) {
- builder.connectionAcquisitionTimeout(
- Duration.ofMillis(httpClientApacheConnectionAcquisitionTimeoutMs));
- }
-
- if (httpClientApacheConnectionMaxIdleTimeMs != null) {
- builder.connectionMaxIdleTime(Duration.ofMillis(httpClientApacheConnectionMaxIdleTimeMs));
- }
-
- if (httpClientApacheConnectionTimeToLiveMs != null) {
- builder.connectionTimeToLive(Duration.ofMillis(httpClientApacheConnectionTimeToLiveMs));
- }
-
- if (httpClientApacheExpectContinueEnabled != null) {
- builder.expectContinueEnabled(httpClientApacheExpectContinueEnabled);
- }
-
- if (httpClientApacheMaxConnections != null) {
- builder.maxConnections(httpClientApacheMaxConnections);
- }
-
- if (httpClientApacheTcpKeepAliveEnabled != null) {
- builder.tcpKeepAlive(httpClientApacheTcpKeepAliveEnabled);
- }
-
- if (httpClientApacheUseIdleConnectionReaperEnabled != null) {
- builder.useIdleConnectionReaper(httpClientApacheUseIdleConnectionReaperEnabled);
+ private HttpClientConfigurations loadHttpClientConfigurations(
+ String impl, Map<String, String> httpClientProperties) {
+ DynConstructors.Ctor<HttpClientConfigurations> ctor;
+ try {
+ ctor =
+ DynConstructors.builder(HttpClientConfigurations.class).hiddenImpl(impl).buildChecked();
+ HttpClientConfigurations httpClientConfigurations = ctor.newInstance();
+ httpClientConfigurations.initialize(httpClientProperties);
+ return httpClientConfigurations;
+ } catch (NoSuchMethodException e) {
+ throw new IllegalArgumentException(
+ String.format(
+ "Cannot initialize HttpClientConfigurations Implementation %s: %s",
+ impl, e.getMessage()),
+ e);
+ } catch (ClassCastException e) {
+ throw new IllegalArgumentException(
+ String.format(
+ "Cannot initialize HttpClientConfigurations, %s does not implement HttpClientConfigurations: %s",
+ impl, e.getMessage()),
Review Comment:
no need to add `e.getMessage` since `e` is already included in the cause.
--
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 #6746: AWS: Load HttpClientBuilder dynamically to avoid runtime deps of both urlconnection and apache client
Posted by "jackye1995 (via GitHub)" <gi...@apache.org>.
jackye1995 merged PR #6746:
URL: https://github.com/apache/iceberg/pull/6746
--
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] stevenzwu commented on a diff in pull request #6746: AWS: Load HttpClientBuilder dynamically to avoid runtime deps of both urlconnection and apache client
Posted by "stevenzwu (via GitHub)" <gi...@apache.org>.
stevenzwu commented on code in PR #6746:
URL: https://github.com/apache/iceberg/pull/6746#discussion_r1096892590
##########
aws/src/main/java/org/apache/iceberg/aws/AwsProperties.java:
##########
@@ -1314,55 +1294,27 @@ private <T extends SdkClientBuilder> void configureEndpoint(T builder, String en
}
}
- @VisibleForTesting
- <T extends UrlConnectionHttpClient.Builder> void configureUrlConnectionHttpClientBuilder(
- T builder) {
- if (httpClientUrlConnectionConnectionTimeoutMs != null) {
- builder.connectionTimeout(Duration.ofMillis(httpClientUrlConnectionConnectionTimeoutMs));
- }
-
- if (httpClientUrlConnectionSocketTimeoutMs != null) {
- builder.socketTimeout(Duration.ofMillis(httpClientUrlConnectionSocketTimeoutMs));
- }
- }
-
- @VisibleForTesting
- <T extends ApacheHttpClient.Builder> void configureApacheHttpClientBuilder(T builder) {
- if (httpClientApacheConnectionTimeoutMs != null) {
- builder.connectionTimeout(Duration.ofMillis(httpClientApacheConnectionTimeoutMs));
- }
-
- if (httpClientApacheSocketTimeoutMs != null) {
- builder.socketTimeout(Duration.ofMillis(httpClientApacheSocketTimeoutMs));
- }
-
- if (httpClientApacheConnectionAcquisitionTimeoutMs != null) {
- builder.connectionAcquisitionTimeout(
- Duration.ofMillis(httpClientApacheConnectionAcquisitionTimeoutMs));
- }
-
- if (httpClientApacheConnectionMaxIdleTimeMs != null) {
- builder.connectionMaxIdleTime(Duration.ofMillis(httpClientApacheConnectionMaxIdleTimeMs));
- }
-
- if (httpClientApacheConnectionTimeToLiveMs != null) {
- builder.connectionTimeToLive(Duration.ofMillis(httpClientApacheConnectionTimeToLiveMs));
- }
-
- if (httpClientApacheExpectContinueEnabled != null) {
- builder.expectContinueEnabled(httpClientApacheExpectContinueEnabled);
- }
-
- if (httpClientApacheMaxConnections != null) {
- builder.maxConnections(httpClientApacheMaxConnections);
- }
-
- if (httpClientApacheTcpKeepAliveEnabled != null) {
- builder.tcpKeepAlive(httpClientApacheTcpKeepAliveEnabled);
- }
-
- if (httpClientApacheUseIdleConnectionReaperEnabled != null) {
- builder.useIdleConnectionReaper(httpClientApacheUseIdleConnectionReaperEnabled);
+ private HttpClientConfigurations loadHttpClientConfigurations(
+ String impl, Map<String, String> httpClientProperties) {
+ DynConstructors.Ctor<HttpClientConfigurations> ctor;
+ try {
+ ctor =
+ DynConstructors.builder(HttpClientConfigurations.class).hiddenImpl(impl).buildChecked();
Review Comment:
I see Jack's point on not exposing a new public interface. I am wondering if we can define `HttpClientConfigurations` as a package private abstract class.
--
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 #6746: AWS: Load HttpClientBuilder dynamically to avoid runtime deps of both urlconnection and apache client
Posted by "jackye1995 (via GitHub)" <gi...@apache.org>.
jackye1995 commented on code in PR #6746:
URL: https://github.com/apache/iceberg/pull/6746#discussion_r1100379158
##########
aws/src/main/java/org/apache/iceberg/aws/AwsProperties.java:
##########
@@ -1314,55 +1270,18 @@ private <T extends SdkClientBuilder> void configureEndpoint(T builder, String en
}
}
- @VisibleForTesting
- <T extends UrlConnectionHttpClient.Builder> void configureUrlConnectionHttpClientBuilder(
- T builder) {
- if (httpClientUrlConnectionConnectionTimeoutMs != null) {
- builder.connectionTimeout(Duration.ofMillis(httpClientUrlConnectionConnectionTimeoutMs));
- }
-
- if (httpClientUrlConnectionSocketTimeoutMs != null) {
- builder.socketTimeout(Duration.ofMillis(httpClientUrlConnectionSocketTimeoutMs));
- }
- }
-
- @VisibleForTesting
- <T extends ApacheHttpClient.Builder> void configureApacheHttpClientBuilder(T builder) {
- if (httpClientApacheConnectionTimeoutMs != null) {
- builder.connectionTimeout(Duration.ofMillis(httpClientApacheConnectionTimeoutMs));
- }
-
- if (httpClientApacheSocketTimeoutMs != null) {
- builder.socketTimeout(Duration.ofMillis(httpClientApacheSocketTimeoutMs));
- }
-
- if (httpClientApacheConnectionAcquisitionTimeoutMs != null) {
- builder.connectionAcquisitionTimeout(
- Duration.ofMillis(httpClientApacheConnectionAcquisitionTimeoutMs));
- }
-
- if (httpClientApacheConnectionMaxIdleTimeMs != null) {
- builder.connectionMaxIdleTime(Duration.ofMillis(httpClientApacheConnectionMaxIdleTimeMs));
- }
-
- if (httpClientApacheConnectionTimeToLiveMs != null) {
- builder.connectionTimeToLive(Duration.ofMillis(httpClientApacheConnectionTimeToLiveMs));
- }
-
- if (httpClientApacheExpectContinueEnabled != null) {
- builder.expectContinueEnabled(httpClientApacheExpectContinueEnabled);
- }
-
- if (httpClientApacheMaxConnections != null) {
- builder.maxConnections(httpClientApacheMaxConnections);
- }
-
- if (httpClientApacheTcpKeepAliveEnabled != null) {
- builder.tcpKeepAlive(httpClientApacheTcpKeepAliveEnabled);
- }
-
- if (httpClientApacheUseIdleConnectionReaperEnabled != null) {
- builder.useIdleConnectionReaper(httpClientApacheUseIdleConnectionReaperEnabled);
+ private Object loadHttpClientConfigurations(String impl) {
+ Object httpClientConfigurations;
+ try {
+ httpClientConfigurations =
+ DynMethods.builder("create")
+ .hiddenImpl(impl, Map.class)
+ .buildStaticChecked()
+ .invoke(httpClientProperties);
+ return httpClientConfigurations;
+ } catch (NoSuchMethodException e) {
+ throw new IllegalArgumentException(
+ String.format("Cannot initialize HttpClientConfigurations Implementation %s", impl), e);
Review Comment:
nit: I think this error message is no longer true? Because there is no HttpClientConfigurations class anymore
--
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 #6746: AWS: Load HttpClientBuilder dynamically to avoid runtime deps of both urlconnection and apache client
Posted by "JonasJ-ap (via GitHub)" <gi...@apache.org>.
JonasJ-ap commented on code in PR #6746:
URL: https://github.com/apache/iceberg/pull/6746#discussion_r1096965051
##########
aws/src/test/java/org/apache/iceberg/aws/TestHttpClientConfigurations.java:
##########
@@ -0,0 +1,404 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.aws;
+
+import java.time.Duration;
+import java.util.Map;
+import org.apache.iceberg.relocated.com.google.common.collect.Maps;
+import org.junit.Assert;
+import org.junit.Test;
+import org.mockito.ArgumentCaptor;
+import org.mockito.Mockito;
+import software.amazon.awssdk.http.apache.ApacheHttpClient;
+import software.amazon.awssdk.http.urlconnection.UrlConnectionHttpClient;
+
+public class TestHttpClientConfigurations {
+ @Test
+ public void testUrlConnectionConnectionSocketTimeoutConfiguration() {
+ Map<String, String> properties = Maps.newHashMap();
+ properties.put(AwsProperties.HTTP_CLIENT_URLCONNECTION_SOCKET_TIMEOUT_MS, "90");
+ properties.put(AwsProperties.HTTP_CLIENT_URLCONNECTION_CONNECTION_TIMEOUT_MS, "80");
+ AwsProperties awsProperties = new AwsProperties(properties);
+ UrlConnectionHttpClientConfigurations urlConnectionHttpClientConfigurations =
+ new UrlConnectionHttpClientConfigurations();
+ urlConnectionHttpClientConfigurations.initialize(
+ awsProperties.urlConnectionHttpClientProperties());
+ UrlConnectionHttpClient.Builder urlConnectionHttpClientBuilder =
+ UrlConnectionHttpClient.builder();
+ UrlConnectionHttpClient.Builder spyUrlConnectionHttpClientBuilder =
+ Mockito.spy(urlConnectionHttpClientBuilder);
+ ArgumentCaptor<Duration> socketTimeoutCaptor = ArgumentCaptor.forClass(Duration.class);
+ ArgumentCaptor<Duration> connectionTimeoutCaptor = ArgumentCaptor.forClass(Duration.class);
+
+ urlConnectionHttpClientConfigurations.configureUrlConnectionHttpClientBuilder(
+ spyUrlConnectionHttpClientBuilder);
+ Mockito.verify(spyUrlConnectionHttpClientBuilder).socketTimeout(socketTimeoutCaptor.capture());
+ Mockito.verify(spyUrlConnectionHttpClientBuilder)
+ .connectionTimeout(connectionTimeoutCaptor.capture());
+
+ Duration capturedSocketTimeout = socketTimeoutCaptor.getValue();
+ Duration capturedConnectionTimeout = connectionTimeoutCaptor.getValue();
+
+ Assert.assertEquals(
+ "The configured socket timeout should be 90 ms", 90, capturedSocketTimeout.toMillis());
+ Assert.assertEquals(
+ "The configured connection timeout should be 80 ms",
+ 80,
+ capturedConnectionTimeout.toMillis());
+ }
+
+ @Test
+ public void testUrlConnectionConnectionTimeoutConfiguration() {
Review Comment:
I think the purpose is to verify that the default behavior is to not configure `socketTimeout` if corresponding property is not set:
```
Mockito.verify(spyUrlConnectionHttpClientBuilder, Mockito.never())
.socketTimeout(socketTimeoutCaptor.capture());
```
I agree that these tests are somewhat redundant. I plan to refactor them to have two tests testing default behavior of all configs for `UrlConnectionHttpClient` and `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 a diff in pull request #6746: AWS: Load HttpClientBuilder dynamically to avoid runtime deps of both urlconnection and apache client
Posted by "jackye1995 (via GitHub)" <gi...@apache.org>.
jackye1995 commented on code in PR #6746:
URL: https://github.com/apache/iceberg/pull/6746#discussion_r1096607340
##########
aws/src/main/java/org/apache/iceberg/aws/AwsProperties.java:
##########
@@ -366,36 +362,48 @@ public class AwsProperties implements Serializable {
*/
public static final String HTTP_CLIENT_TYPE_URLCONNECTION = "urlconnection";
+ /**
+ * The prefix for configurations of for {@link
+ * software.amazon.awssdk.http.urlconnection.UrlConnectionHttpClient.Builder}
+ */
+ public static final String HTTP_CLIENT_URLCONNECTION_PREFIX = "http-client.urlconnection.";
+
/**
* If this is set under {@link #HTTP_CLIENT_TYPE}, {@link
* software.amazon.awssdk.http.apache.ApacheHttpClient} will be used as the HTTP Client in {@link
* AwsClientFactory}
*/
public static final String HTTP_CLIENT_TYPE_APACHE = "apache";
+ /**
+ * The prefix for configurations of for {@link
+ * software.amazon.awssdk.http.apache.ApacheHttpClient.Builder}
+ */
+ public static final String HTTP_CLIENT_APACHE_PREFIX = "http-client.apache.";
+
public static final String HTTP_CLIENT_TYPE_DEFAULT = HTTP_CLIENT_TYPE_URLCONNECTION;
/**
* Used to configure the connection timeout in milliseconds for {@link
- * UrlConnectionHttpClient.Builder}. This flag only works when {@link #HTTP_CLIENT_TYPE} is set to
- * {@link #HTTP_CLIENT_TYPE_URLCONNECTION}
+ * software.amazon.awssdk.http.urlconnection.UrlConnectionHttpClient.Builder}. This flag only
+ * works when {@link #HTTP_CLIENT_TYPE} is set to {@link #HTTP_CLIENT_TYPE_URLCONNECTION}
*
* <p>For more details, see
* https://sdk.amazonaws.com/java/api/latest/software/amazon/awssdk/http/urlconnection/UrlConnectionHttpClient.Builder.html
*/
public static final String HTTP_CLIENT_URLCONNECTION_CONNECTION_TIMEOUT_MS =
- "http-client.urlconnection.connection-timeout-ms";
+ HTTP_CLIENT_URLCONNECTION_PREFIX + HttpClientConfigurations.CONNECTION_TIMEOUT_MS;
Review Comment:
I remember we preferred to use the full config name because it is easier for people to search for it in the codebase
--
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 #6746: AWS: Load HttpClientBuilder dynamically to avoid runtime deps of both urlconnection and apache client
Posted by "jackye1995 (via GitHub)" <gi...@apache.org>.
jackye1995 commented on code in PR #6746:
URL: https://github.com/apache/iceberg/pull/6746#discussion_r1097868200
##########
aws/src/main/java/org/apache/iceberg/aws/ApacheHttpClientConfigurations.java:
##########
@@ -0,0 +1,109 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.aws;
+
+import java.time.Duration;
+import java.util.Map;
+import org.apache.iceberg.relocated.com.google.common.annotations.VisibleForTesting;
+import org.apache.iceberg.util.PropertyUtil;
+import software.amazon.awssdk.awscore.client.builder.AwsSyncClientBuilder;
+import software.amazon.awssdk.http.apache.ApacheHttpClient;
+
+class ApacheHttpClientConfigurations extends HttpClientConfigurations {
Review Comment:
oh nvm, they need to be in separated class files to avoid loading them together
--
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 #6746: AWS: Load HttpClientBuilder dynamically to avoid runtime deps of both urlconnection and apache client
Posted by "jackye1995 (via GitHub)" <gi...@apache.org>.
jackye1995 commented on PR #6746:
URL: https://github.com/apache/iceberg/pull/6746#issuecomment-1424575793
I think all the comments are addressed and we have enough vote, I will go ahead and merge this. Thanks for fixing this with such detailed verification! And thanks for the review @stevenzwu @rdblue
--
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] stevenzwu commented on a diff in pull request #6746: AWS: Load HttpClientBuilder dynamically to avoid runtime deps of both urlconnection and apache client
Posted by "stevenzwu (via GitHub)" <gi...@apache.org>.
stevenzwu commented on code in PR #6746:
URL: https://github.com/apache/iceberg/pull/6746#discussion_r1097776431
##########
aws/src/main/java/org/apache/iceberg/aws/AwsProperties.java:
##########
@@ -1314,55 +1294,27 @@ private <T extends SdkClientBuilder> void configureEndpoint(T builder, String en
}
}
- @VisibleForTesting
- <T extends UrlConnectionHttpClient.Builder> void configureUrlConnectionHttpClientBuilder(
- T builder) {
- if (httpClientUrlConnectionConnectionTimeoutMs != null) {
- builder.connectionTimeout(Duration.ofMillis(httpClientUrlConnectionConnectionTimeoutMs));
- }
-
- if (httpClientUrlConnectionSocketTimeoutMs != null) {
- builder.socketTimeout(Duration.ofMillis(httpClientUrlConnectionSocketTimeoutMs));
- }
- }
-
- @VisibleForTesting
- <T extends ApacheHttpClient.Builder> void configureApacheHttpClientBuilder(T builder) {
- if (httpClientApacheConnectionTimeoutMs != null) {
- builder.connectionTimeout(Duration.ofMillis(httpClientApacheConnectionTimeoutMs));
- }
-
- if (httpClientApacheSocketTimeoutMs != null) {
- builder.socketTimeout(Duration.ofMillis(httpClientApacheSocketTimeoutMs));
- }
-
- if (httpClientApacheConnectionAcquisitionTimeoutMs != null) {
- builder.connectionAcquisitionTimeout(
- Duration.ofMillis(httpClientApacheConnectionAcquisitionTimeoutMs));
- }
-
- if (httpClientApacheConnectionMaxIdleTimeMs != null) {
- builder.connectionMaxIdleTime(Duration.ofMillis(httpClientApacheConnectionMaxIdleTimeMs));
- }
-
- if (httpClientApacheConnectionTimeToLiveMs != null) {
- builder.connectionTimeToLive(Duration.ofMillis(httpClientApacheConnectionTimeToLiveMs));
- }
-
- if (httpClientApacheExpectContinueEnabled != null) {
- builder.expectContinueEnabled(httpClientApacheExpectContinueEnabled);
- }
-
- if (httpClientApacheMaxConnections != null) {
- builder.maxConnections(httpClientApacheMaxConnections);
- }
-
- if (httpClientApacheTcpKeepAliveEnabled != null) {
- builder.tcpKeepAlive(httpClientApacheTcpKeepAliveEnabled);
- }
-
- if (httpClientApacheUseIdleConnectionReaperEnabled != null) {
- builder.useIdleConnectionReaper(httpClientApacheUseIdleConnectionReaperEnabled);
+ private HttpClientConfigurations loadHttpClientConfigurations(
+ String impl, Map<String, String> httpClientProperties) {
+ DynConstructors.Ctor<HttpClientConfigurations> ctor;
+ try {
+ ctor =
+ DynConstructors.builder(HttpClientConfigurations.class).hiddenImpl(impl).buildChecked();
Review Comment:
good question. it is a miss from my end. I just assumed it is a public interface. Yeah, I agree that package-private interface should be same.
--
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 #6746: AWS: Load HttpClientBuilder dynamically to avoid runtime deps of both urlconnection and apache client
Posted by "jackye1995 (via GitHub)" <gi...@apache.org>.
jackye1995 commented on code in PR #6746:
URL: https://github.com/apache/iceberg/pull/6746#discussion_r1096608183
##########
aws/src/main/java/org/apache/iceberg/aws/AwsProperties.java:
##########
@@ -1314,55 +1294,27 @@ private <T extends SdkClientBuilder> void configureEndpoint(T builder, String en
}
}
- @VisibleForTesting
- <T extends UrlConnectionHttpClient.Builder> void configureUrlConnectionHttpClientBuilder(
- T builder) {
- if (httpClientUrlConnectionConnectionTimeoutMs != null) {
- builder.connectionTimeout(Duration.ofMillis(httpClientUrlConnectionConnectionTimeoutMs));
- }
-
- if (httpClientUrlConnectionSocketTimeoutMs != null) {
- builder.socketTimeout(Duration.ofMillis(httpClientUrlConnectionSocketTimeoutMs));
- }
- }
-
- @VisibleForTesting
- <T extends ApacheHttpClient.Builder> void configureApacheHttpClientBuilder(T builder) {
- if (httpClientApacheConnectionTimeoutMs != null) {
- builder.connectionTimeout(Duration.ofMillis(httpClientApacheConnectionTimeoutMs));
- }
-
- if (httpClientApacheSocketTimeoutMs != null) {
- builder.socketTimeout(Duration.ofMillis(httpClientApacheSocketTimeoutMs));
- }
-
- if (httpClientApacheConnectionAcquisitionTimeoutMs != null) {
- builder.connectionAcquisitionTimeout(
- Duration.ofMillis(httpClientApacheConnectionAcquisitionTimeoutMs));
- }
-
- if (httpClientApacheConnectionMaxIdleTimeMs != null) {
- builder.connectionMaxIdleTime(Duration.ofMillis(httpClientApacheConnectionMaxIdleTimeMs));
- }
-
- if (httpClientApacheConnectionTimeToLiveMs != null) {
- builder.connectionTimeToLive(Duration.ofMillis(httpClientApacheConnectionTimeToLiveMs));
- }
-
- if (httpClientApacheExpectContinueEnabled != null) {
- builder.expectContinueEnabled(httpClientApacheExpectContinueEnabled);
- }
-
- if (httpClientApacheMaxConnections != null) {
- builder.maxConnections(httpClientApacheMaxConnections);
- }
-
- if (httpClientApacheTcpKeepAliveEnabled != null) {
- builder.tcpKeepAlive(httpClientApacheTcpKeepAliveEnabled);
- }
-
- if (httpClientApacheUseIdleConnectionReaperEnabled != null) {
- builder.useIdleConnectionReaper(httpClientApacheUseIdleConnectionReaperEnabled);
+ private HttpClientConfigurations loadHttpClientConfigurations(
+ String impl, Map<String, String> httpClientProperties) {
+ DynConstructors.Ctor<HttpClientConfigurations> ctor;
+ try {
+ ctor =
+ DynConstructors.builder(HttpClientConfigurations.class).hiddenImpl(impl).buildChecked();
Review Comment:
trying to see if we can avoid having to create a new public interface like `HttpClientConfigurations`. We can have some static methods in protected class, and dynamically load them using something like
```
DynMethods.builder("initialize")
.impl(impl, Map.class)
.buildStaticChecked()
```
--
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 #6746: AWS: Load HttpClientBuilder dynamically to avoid runtime deps of both urlconnection and apache client
Posted by "jackye1995 (via GitHub)" <gi...@apache.org>.
jackye1995 commented on code in PR #6746:
URL: https://github.com/apache/iceberg/pull/6746#discussion_r1096608183
##########
aws/src/main/java/org/apache/iceberg/aws/AwsProperties.java:
##########
@@ -1314,55 +1294,27 @@ private <T extends SdkClientBuilder> void configureEndpoint(T builder, String en
}
}
- @VisibleForTesting
- <T extends UrlConnectionHttpClient.Builder> void configureUrlConnectionHttpClientBuilder(
- T builder) {
- if (httpClientUrlConnectionConnectionTimeoutMs != null) {
- builder.connectionTimeout(Duration.ofMillis(httpClientUrlConnectionConnectionTimeoutMs));
- }
-
- if (httpClientUrlConnectionSocketTimeoutMs != null) {
- builder.socketTimeout(Duration.ofMillis(httpClientUrlConnectionSocketTimeoutMs));
- }
- }
-
- @VisibleForTesting
- <T extends ApacheHttpClient.Builder> void configureApacheHttpClientBuilder(T builder) {
- if (httpClientApacheConnectionTimeoutMs != null) {
- builder.connectionTimeout(Duration.ofMillis(httpClientApacheConnectionTimeoutMs));
- }
-
- if (httpClientApacheSocketTimeoutMs != null) {
- builder.socketTimeout(Duration.ofMillis(httpClientApacheSocketTimeoutMs));
- }
-
- if (httpClientApacheConnectionAcquisitionTimeoutMs != null) {
- builder.connectionAcquisitionTimeout(
- Duration.ofMillis(httpClientApacheConnectionAcquisitionTimeoutMs));
- }
-
- if (httpClientApacheConnectionMaxIdleTimeMs != null) {
- builder.connectionMaxIdleTime(Duration.ofMillis(httpClientApacheConnectionMaxIdleTimeMs));
- }
-
- if (httpClientApacheConnectionTimeToLiveMs != null) {
- builder.connectionTimeToLive(Duration.ofMillis(httpClientApacheConnectionTimeToLiveMs));
- }
-
- if (httpClientApacheExpectContinueEnabled != null) {
- builder.expectContinueEnabled(httpClientApacheExpectContinueEnabled);
- }
-
- if (httpClientApacheMaxConnections != null) {
- builder.maxConnections(httpClientApacheMaxConnections);
- }
-
- if (httpClientApacheTcpKeepAliveEnabled != null) {
- builder.tcpKeepAlive(httpClientApacheTcpKeepAliveEnabled);
- }
-
- if (httpClientApacheUseIdleConnectionReaperEnabled != null) {
- builder.useIdleConnectionReaper(httpClientApacheUseIdleConnectionReaperEnabled);
+ private HttpClientConfigurations loadHttpClientConfigurations(
+ String impl, Map<String, String> httpClientProperties) {
+ DynConstructors.Ctor<HttpClientConfigurations> ctor;
+ try {
+ ctor =
+ DynConstructors.builder(HttpClientConfigurations.class).hiddenImpl(impl).buildChecked();
Review Comment:
trying to see if we can avoid having to create a new public interface like `HttpClientConfigurations`. We can have some static methods in non-public class, and dynamically load them using something like
```
DynMethods.builder("initialize")
.impl(impl, Map.class)
.buildStaticChecked()
```
--
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 #6746: AWS: Load HttpClientBuilder dynamically to avoid runtime deps of both urlconnection and apache client
Posted by "JonasJ-ap (via GitHub)" <gi...@apache.org>.
JonasJ-ap commented on code in PR #6746:
URL: https://github.com/apache/iceberg/pull/6746#discussion_r1096940112
##########
aws/src/main/java/org/apache/iceberg/aws/AwsProperties.java:
##########
@@ -366,36 +362,48 @@ public class AwsProperties implements Serializable {
*/
public static final String HTTP_CLIENT_TYPE_URLCONNECTION = "urlconnection";
+ /**
+ * The prefix for configurations of for {@link
+ * software.amazon.awssdk.http.urlconnection.UrlConnectionHttpClient.Builder}
+ */
+ public static final String HTTP_CLIENT_URLCONNECTION_PREFIX = "http-client.urlconnection.";
+
/**
* If this is set under {@link #HTTP_CLIENT_TYPE}, {@link
* software.amazon.awssdk.http.apache.ApacheHttpClient} will be used as the HTTP Client in {@link
* AwsClientFactory}
*/
public static final String HTTP_CLIENT_TYPE_APACHE = "apache";
+ /**
+ * The prefix for configurations of for {@link
+ * software.amazon.awssdk.http.apache.ApacheHttpClient.Builder}
+ */
+ public static final String HTTP_CLIENT_APACHE_PREFIX = "http-client.apache.";
+
public static final String HTTP_CLIENT_TYPE_DEFAULT = HTTP_CLIENT_TYPE_URLCONNECTION;
/**
* Used to configure the connection timeout in milliseconds for {@link
- * UrlConnectionHttpClient.Builder}. This flag only works when {@link #HTTP_CLIENT_TYPE} is set to
- * {@link #HTTP_CLIENT_TYPE_URLCONNECTION}
+ * software.amazon.awssdk.http.urlconnection.UrlConnectionHttpClient.Builder}. This flag only
+ * works when {@link #HTTP_CLIENT_TYPE} is set to {@link #HTTP_CLIENT_TYPE_URLCONNECTION}
*
* <p>For more details, see
* https://sdk.amazonaws.com/java/api/latest/software/amazon/awssdk/http/urlconnection/UrlConnectionHttpClient.Builder.html
*/
public static final String HTTP_CLIENT_URLCONNECTION_CONNECTION_TIMEOUT_MS =
- "http-client.urlconnection.connection-timeout-ms";
+ HTTP_CLIENT_URLCONNECTION_PREFIX + HttpClientConfigurations.CONNECTION_TIMEOUT_MS;
Review Comment:
Thank you for your suggestions. I've restored them to full 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] stevenzwu commented on a diff in pull request #6746: AWS: Load HttpClientBuilder dynamically to avoid runtime deps of both urlconnection and apache client
Posted by "stevenzwu (via GitHub)" <gi...@apache.org>.
stevenzwu commented on code in PR #6746:
URL: https://github.com/apache/iceberg/pull/6746#discussion_r1096950110
##########
aws/src/test/java/org/apache/iceberg/aws/TestHttpClientConfigurations.java:
##########
@@ -0,0 +1,404 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.aws;
+
+import java.time.Duration;
+import java.util.Map;
+import org.apache.iceberg.relocated.com.google.common.collect.Maps;
+import org.junit.Assert;
+import org.junit.Test;
+import org.mockito.ArgumentCaptor;
+import org.mockito.Mockito;
+import software.amazon.awssdk.http.apache.ApacheHttpClient;
+import software.amazon.awssdk.http.urlconnection.UrlConnectionHttpClient;
+
+public class TestHttpClientConfigurations {
+ @Test
+ public void testUrlConnectionConnectionSocketTimeoutConfiguration() {
+ Map<String, String> properties = Maps.newHashMap();
+ properties.put(AwsProperties.HTTP_CLIENT_URLCONNECTION_SOCKET_TIMEOUT_MS, "90");
+ properties.put(AwsProperties.HTTP_CLIENT_URLCONNECTION_CONNECTION_TIMEOUT_MS, "80");
+ AwsProperties awsProperties = new AwsProperties(properties);
+ UrlConnectionHttpClientConfigurations urlConnectionHttpClientConfigurations =
+ new UrlConnectionHttpClientConfigurations();
+ urlConnectionHttpClientConfigurations.initialize(
+ awsProperties.urlConnectionHttpClientProperties());
+ UrlConnectionHttpClient.Builder urlConnectionHttpClientBuilder =
+ UrlConnectionHttpClient.builder();
+ UrlConnectionHttpClient.Builder spyUrlConnectionHttpClientBuilder =
+ Mockito.spy(urlConnectionHttpClientBuilder);
+ ArgumentCaptor<Duration> socketTimeoutCaptor = ArgumentCaptor.forClass(Duration.class);
+ ArgumentCaptor<Duration> connectionTimeoutCaptor = ArgumentCaptor.forClass(Duration.class);
+
+ urlConnectionHttpClientConfigurations.configureUrlConnectionHttpClientBuilder(
+ spyUrlConnectionHttpClientBuilder);
+ Mockito.verify(spyUrlConnectionHttpClientBuilder).socketTimeout(socketTimeoutCaptor.capture());
+ Mockito.verify(spyUrlConnectionHttpClientBuilder)
+ .connectionTimeout(connectionTimeoutCaptor.capture());
+
+ Duration capturedSocketTimeout = socketTimeoutCaptor.getValue();
+ Duration capturedConnectionTimeout = connectionTimeoutCaptor.getValue();
+
+ Assert.assertEquals(
+ "The configured socket timeout should be 90 ms", 90, capturedSocketTimeout.toMillis());
+ Assert.assertEquals(
+ "The configured connection timeout should be 80 ms",
+ 80,
+ capturedConnectionTimeout.toMillis());
+ }
+
+ @Test
+ public void testUrlConnectionConnectionTimeoutConfiguration() {
Review Comment:
ok. I see it is just refactored over from `TestAwsProperties`. still find it a little weird to test `override 2 configs` vs `override 1 config`. anyway, this is fine by me.
--
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 #6746: AWS: Load HttpClientBuilder dynamically to avoid runtime deps of both urlconnection and apache client
Posted by "jackye1995 (via GitHub)" <gi...@apache.org>.
jackye1995 commented on code in PR #6746:
URL: https://github.com/apache/iceberg/pull/6746#discussion_r1096607921
##########
aws/src/main/java/org/apache/iceberg/aws/AwsProperties.java:
##########
@@ -366,36 +362,48 @@ public class AwsProperties implements Serializable {
*/
public static final String HTTP_CLIENT_TYPE_URLCONNECTION = "urlconnection";
+ /**
+ * The prefix for configurations of for {@link
+ * software.amazon.awssdk.http.urlconnection.UrlConnectionHttpClient.Builder}
+ */
+ public static final String HTTP_CLIENT_URLCONNECTION_PREFIX = "http-client.urlconnection.";
+
/**
* If this is set under {@link #HTTP_CLIENT_TYPE}, {@link
* software.amazon.awssdk.http.apache.ApacheHttpClient} will be used as the HTTP Client in {@link
* AwsClientFactory}
*/
public static final String HTTP_CLIENT_TYPE_APACHE = "apache";
+ /**
+ * The prefix for configurations of for {@link
+ * software.amazon.awssdk.http.apache.ApacheHttpClient.Builder}
+ */
+ public static final String HTTP_CLIENT_APACHE_PREFIX = "http-client.apache.";
+
public static final String HTTP_CLIENT_TYPE_DEFAULT = HTTP_CLIENT_TYPE_URLCONNECTION;
/**
* Used to configure the connection timeout in milliseconds for {@link
- * UrlConnectionHttpClient.Builder}. This flag only works when {@link #HTTP_CLIENT_TYPE} is set to
- * {@link #HTTP_CLIENT_TYPE_URLCONNECTION}
+ * software.amazon.awssdk.http.urlconnection.UrlConnectionHttpClient.Builder}. This flag only
+ * works when {@link #HTTP_CLIENT_TYPE} is set to {@link #HTTP_CLIENT_TYPE_URLCONNECTION}
*
* <p>For more details, see
* https://sdk.amazonaws.com/java/api/latest/software/amazon/awssdk/http/urlconnection/UrlConnectionHttpClient.Builder.html
*/
public static final String HTTP_CLIENT_URLCONNECTION_CONNECTION_TIMEOUT_MS =
- "http-client.urlconnection.connection-timeout-ms";
+ HTTP_CLIENT_URLCONNECTION_PREFIX + HttpClientConfigurations.CONNECTION_TIMEOUT_MS;
Review Comment:
So what we can do is that, we can keep these strings as is, and then instead of using `propertiesWithPrefix`, we can just pass in the original property map to initialize the client
--
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 #6746: AWS: Load HttpClientBuilder dynamically to avoid runtime deps of both urlconnection and apache client
Posted by "jackye1995 (via GitHub)" <gi...@apache.org>.
jackye1995 commented on code in PR #6746:
URL: https://github.com/apache/iceberg/pull/6746#discussion_r1097864293
##########
aws/src/main/java/org/apache/iceberg/aws/ApacheHttpClientConfigurations.java:
##########
@@ -0,0 +1,109 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.aws;
+
+import java.time.Duration;
+import java.util.Map;
+import org.apache.iceberg.relocated.com.google.common.annotations.VisibleForTesting;
+import org.apache.iceberg.util.PropertyUtil;
+import software.amazon.awssdk.awscore.client.builder.AwsSyncClientBuilder;
+import software.amazon.awssdk.http.apache.ApacheHttpClient;
+
+class ApacheHttpClientConfigurations extends HttpClientConfigurations {
Review Comment:
nit: can we make these inner classes under `HttpClientConfigurations` so that we don't introduce too many classes?
--
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 #6746: AWS: Load HttpClientBuilder dynamically to avoid runtime deps of both urlconnection and apache client
Posted by "jackye1995 (via GitHub)" <gi...@apache.org>.
jackye1995 commented on code in PR #6746:
URL: https://github.com/apache/iceberg/pull/6746#discussion_r1097866781
##########
aws/src/main/java/org/apache/iceberg/aws/AwsProperties.java:
##########
@@ -632,18 +628,11 @@ public class AwsProperties implements Serializable {
*/
public static final String LAKE_FORMATION_DB_NAME = "lakeformation.db-name";
+ private static final String HTTP_CLIENT_URLCONNECTION_PREFIX = "http-client.urlconnection.";
+ private static final String HTTP_CLIENT_APACHE_PREFIX = "http-client.apache.";
private String httpClientType;
- private Long httpClientUrlConnectionConnectionTimeoutMs;
- private Long httpClientUrlConnectionSocketTimeoutMs;
- private Long httpClientApacheConnectionAcquisitionTimeoutMs;
- private Long httpClientApacheConnectionMaxIdleTimeMs;
- private Long httpClientApacheConnectionTimeToLiveMs;
- private Long httpClientApacheConnectionTimeoutMs;
- private Boolean httpClientApacheExpectContinueEnabled;
- private Integer httpClientApacheMaxConnections;
- private Long httpClientApacheSocketTimeoutMs;
- private Boolean httpClientApacheTcpKeepAliveEnabled;
- private Boolean httpClientApacheUseIdleConnectionReaperEnabled;
+ private final Map<String, String> urlConnectionHttpClientProperties;
Review Comment:
I think we can avoid having 2 maps by storing just fields with prefix `http-client.`
--
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 #6746: AWS: Load HttpClientBuilder dynamically to avoid runtime deps of both urlconnection and apache client
Posted by "JonasJ-ap (via GitHub)" <gi...@apache.org>.
JonasJ-ap commented on PR #6746:
URL: https://github.com/apache/iceberg/pull/6746#issuecomment-1421709404
Thanks everyone for reviewing this! I will add a proof of running the latest update on EKS later today or tomorrow
--
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 #6746: AWS: Load HttpClientBuilder dynamically to avoid runtime deps of both urlconnection and apache client
Posted by "jackye1995 (via GitHub)" <gi...@apache.org>.
jackye1995 commented on code in PR #6746:
URL: https://github.com/apache/iceberg/pull/6746#discussion_r1097870631
##########
aws/src/main/java/org/apache/iceberg/aws/AwsProperties.java:
##########
@@ -1314,55 +1294,27 @@ private <T extends SdkClientBuilder> void configureEndpoint(T builder, String en
}
}
- @VisibleForTesting
- <T extends UrlConnectionHttpClient.Builder> void configureUrlConnectionHttpClientBuilder(
- T builder) {
- if (httpClientUrlConnectionConnectionTimeoutMs != null) {
- builder.connectionTimeout(Duration.ofMillis(httpClientUrlConnectionConnectionTimeoutMs));
- }
-
- if (httpClientUrlConnectionSocketTimeoutMs != null) {
- builder.socketTimeout(Duration.ofMillis(httpClientUrlConnectionSocketTimeoutMs));
- }
- }
-
- @VisibleForTesting
- <T extends ApacheHttpClient.Builder> void configureApacheHttpClientBuilder(T builder) {
- if (httpClientApacheConnectionTimeoutMs != null) {
- builder.connectionTimeout(Duration.ofMillis(httpClientApacheConnectionTimeoutMs));
- }
-
- if (httpClientApacheSocketTimeoutMs != null) {
- builder.socketTimeout(Duration.ofMillis(httpClientApacheSocketTimeoutMs));
- }
-
- if (httpClientApacheConnectionAcquisitionTimeoutMs != null) {
- builder.connectionAcquisitionTimeout(
- Duration.ofMillis(httpClientApacheConnectionAcquisitionTimeoutMs));
- }
-
- if (httpClientApacheConnectionMaxIdleTimeMs != null) {
- builder.connectionMaxIdleTime(Duration.ofMillis(httpClientApacheConnectionMaxIdleTimeMs));
- }
-
- if (httpClientApacheConnectionTimeToLiveMs != null) {
- builder.connectionTimeToLive(Duration.ofMillis(httpClientApacheConnectionTimeToLiveMs));
- }
-
- if (httpClientApacheExpectContinueEnabled != null) {
- builder.expectContinueEnabled(httpClientApacheExpectContinueEnabled);
- }
-
- if (httpClientApacheMaxConnections != null) {
- builder.maxConnections(httpClientApacheMaxConnections);
- }
-
- if (httpClientApacheTcpKeepAliveEnabled != null) {
- builder.tcpKeepAlive(httpClientApacheTcpKeepAliveEnabled);
- }
-
- if (httpClientApacheUseIdleConnectionReaperEnabled != null) {
- builder.useIdleConnectionReaper(httpClientApacheUseIdleConnectionReaperEnabled);
+ private HttpClientConfigurations loadHttpClientConfigurations(
+ String impl, Map<String, String> httpClientProperties) {
+ DynConstructors.Ctor<HttpClientConfigurations> ctor;
+ try {
+ ctor =
+ DynConstructors.builder(HttpClientConfigurations.class).hiddenImpl(impl).buildChecked();
Review Comment:
The abstract class looks good to me. But why not just having a static method though? In that case we completely save the need for them to inherit the same abstract class.
--
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] rdblue commented on a diff in pull request #6746: AWS: Load HttpClientBuilder dynamically to avoid runtime deps of both urlconnection and apache client
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #6746:
URL: https://github.com/apache/iceberg/pull/6746#discussion_r1097905950
##########
core/src/main/java/org/apache/iceberg/util/PropertyUtil.java:
##########
@@ -120,6 +120,27 @@ public static Map<String, String> propertiesWithPrefix(
.collect(Collectors.toMap(e -> e.getKey().replaceFirst(prefix, ""), Map.Entry::getValue));
}
+ /**
+ * Returns subset of provided map with keys matching the provided prefix. Matching is
+ * case-sensitive.
+ *
+ * @param properties input map
+ * @param prefix prefix to choose keys from input map
+ * @return subset of input map with keys starting with provided prefix
+ */
+ public static Map<String, String> propertiesWithPrefixNoTrim(
Review Comment:
"Trim" typically means removing whitespace. Is there a better way to name this method? Maybe the `startsWith` could be passed as a `Predicate<String>` and this could be `filterProperties`? Or maybe `filterPropertiesByPrefix`?
--
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 #6746: AWS: Load HttpClientBuilder dynamically to avoid runtime deps of both urlconnection and apache client
Posted by "jackye1995 (via GitHub)" <gi...@apache.org>.
jackye1995 commented on code in PR #6746:
URL: https://github.com/apache/iceberg/pull/6746#discussion_r1100376899
##########
aws/src/main/java/org/apache/iceberg/aws/AwsProperties.java:
##########
@@ -1314,55 +1270,18 @@ private <T extends SdkClientBuilder> void configureEndpoint(T builder, String en
}
}
- @VisibleForTesting
- <T extends UrlConnectionHttpClient.Builder> void configureUrlConnectionHttpClientBuilder(
- T builder) {
- if (httpClientUrlConnectionConnectionTimeoutMs != null) {
- builder.connectionTimeout(Duration.ofMillis(httpClientUrlConnectionConnectionTimeoutMs));
- }
-
- if (httpClientUrlConnectionSocketTimeoutMs != null) {
- builder.socketTimeout(Duration.ofMillis(httpClientUrlConnectionSocketTimeoutMs));
- }
- }
-
- @VisibleForTesting
- <T extends ApacheHttpClient.Builder> void configureApacheHttpClientBuilder(T builder) {
- if (httpClientApacheConnectionTimeoutMs != null) {
- builder.connectionTimeout(Duration.ofMillis(httpClientApacheConnectionTimeoutMs));
- }
-
- if (httpClientApacheSocketTimeoutMs != null) {
- builder.socketTimeout(Duration.ofMillis(httpClientApacheSocketTimeoutMs));
- }
-
- if (httpClientApacheConnectionAcquisitionTimeoutMs != null) {
- builder.connectionAcquisitionTimeout(
- Duration.ofMillis(httpClientApacheConnectionAcquisitionTimeoutMs));
- }
-
- if (httpClientApacheConnectionMaxIdleTimeMs != null) {
- builder.connectionMaxIdleTime(Duration.ofMillis(httpClientApacheConnectionMaxIdleTimeMs));
- }
-
- if (httpClientApacheConnectionTimeToLiveMs != null) {
- builder.connectionTimeToLive(Duration.ofMillis(httpClientApacheConnectionTimeToLiveMs));
- }
-
- if (httpClientApacheExpectContinueEnabled != null) {
- builder.expectContinueEnabled(httpClientApacheExpectContinueEnabled);
- }
-
- if (httpClientApacheMaxConnections != null) {
- builder.maxConnections(httpClientApacheMaxConnections);
- }
-
- if (httpClientApacheTcpKeepAliveEnabled != null) {
- builder.tcpKeepAlive(httpClientApacheTcpKeepAliveEnabled);
- }
-
- if (httpClientApacheUseIdleConnectionReaperEnabled != null) {
- builder.useIdleConnectionReaper(httpClientApacheUseIdleConnectionReaperEnabled);
+ private Object loadHttpClientConfigurations(String impl) {
Review Comment:
+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
[GitHub] [iceberg] stevenzwu commented on a diff in pull request #6746: AWS: Load HttpClientBuilder dynamically to avoid runtime deps of both urlconnection and apache client
Posted by "stevenzwu (via GitHub)" <gi...@apache.org>.
stevenzwu commented on code in PR #6746:
URL: https://github.com/apache/iceberg/pull/6746#discussion_r1096886387
##########
aws/src/main/java/org/apache/iceberg/aws/AwsProperties.java:
##########
@@ -366,36 +362,48 @@ public class AwsProperties implements Serializable {
*/
public static final String HTTP_CLIENT_TYPE_URLCONNECTION = "urlconnection";
+ /**
+ * The prefix for configurations of for {@link
+ * software.amazon.awssdk.http.urlconnection.UrlConnectionHttpClient.Builder}
+ */
+ public static final String HTTP_CLIENT_URLCONNECTION_PREFIX = "http-client.urlconnection.";
+
/**
* If this is set under {@link #HTTP_CLIENT_TYPE}, {@link
* software.amazon.awssdk.http.apache.ApacheHttpClient} will be used as the HTTP Client in {@link
* AwsClientFactory}
*/
public static final String HTTP_CLIENT_TYPE_APACHE = "apache";
+ /**
+ * The prefix for configurations of for {@link
+ * software.amazon.awssdk.http.apache.ApacheHttpClient.Builder}
+ */
+ public static final String HTTP_CLIENT_APACHE_PREFIX = "http-client.apache.";
+
public static final String HTTP_CLIENT_TYPE_DEFAULT = HTTP_CLIENT_TYPE_URLCONNECTION;
/**
* Used to configure the connection timeout in milliseconds for {@link
- * UrlConnectionHttpClient.Builder}. This flag only works when {@link #HTTP_CLIENT_TYPE} is set to
- * {@link #HTTP_CLIENT_TYPE_URLCONNECTION}
+ * software.amazon.awssdk.http.urlconnection.UrlConnectionHttpClient.Builder}. This flag only
+ * works when {@link #HTTP_CLIENT_TYPE} is set to {@link #HTTP_CLIENT_TYPE_URLCONNECTION}
*
* <p>For more details, see
* https://sdk.amazonaws.com/java/api/latest/software/amazon/awssdk/http/urlconnection/UrlConnectionHttpClient.Builder.html
*/
public static final String HTTP_CLIENT_URLCONNECTION_CONNECTION_TIMEOUT_MS =
- "http-client.urlconnection.connection-timeout-ms";
+ HTTP_CLIENT_URLCONNECTION_PREFIX + HttpClientConfigurations.CONNECTION_TIMEOUT_MS;
Review Comment:
totally agree with @jackye1995 here.
--
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] stevenzwu commented on a diff in pull request #6746: AWS: Load HttpClientBuilder dynamically to avoid runtime deps of both urlconnection and apache client
Posted by "stevenzwu (via GitHub)" <gi...@apache.org>.
stevenzwu commented on code in PR #6746:
URL: https://github.com/apache/iceberg/pull/6746#discussion_r1096949257
##########
aws/src/test/java/org/apache/iceberg/aws/TestHttpClientConfigurations.java:
##########
@@ -0,0 +1,404 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.aws;
+
+import java.time.Duration;
+import java.util.Map;
+import org.apache.iceberg.relocated.com.google.common.collect.Maps;
+import org.junit.Assert;
+import org.junit.Test;
+import org.mockito.ArgumentCaptor;
+import org.mockito.Mockito;
+import software.amazon.awssdk.http.apache.ApacheHttpClient;
+import software.amazon.awssdk.http.urlconnection.UrlConnectionHttpClient;
+
+public class TestHttpClientConfigurations {
+ @Test
+ public void testUrlConnectionConnectionSocketTimeoutConfiguration() {
+ Map<String, String> properties = Maps.newHashMap();
+ properties.put(AwsProperties.HTTP_CLIENT_URLCONNECTION_SOCKET_TIMEOUT_MS, "90");
+ properties.put(AwsProperties.HTTP_CLIENT_URLCONNECTION_CONNECTION_TIMEOUT_MS, "80");
+ AwsProperties awsProperties = new AwsProperties(properties);
+ UrlConnectionHttpClientConfigurations urlConnectionHttpClientConfigurations =
+ new UrlConnectionHttpClientConfigurations();
+ urlConnectionHttpClientConfigurations.initialize(
+ awsProperties.urlConnectionHttpClientProperties());
+ UrlConnectionHttpClient.Builder urlConnectionHttpClientBuilder =
+ UrlConnectionHttpClient.builder();
+ UrlConnectionHttpClient.Builder spyUrlConnectionHttpClientBuilder =
+ Mockito.spy(urlConnectionHttpClientBuilder);
+ ArgumentCaptor<Duration> socketTimeoutCaptor = ArgumentCaptor.forClass(Duration.class);
+ ArgumentCaptor<Duration> connectionTimeoutCaptor = ArgumentCaptor.forClass(Duration.class);
+
+ urlConnectionHttpClientConfigurations.configureUrlConnectionHttpClientBuilder(
+ spyUrlConnectionHttpClientBuilder);
+ Mockito.verify(spyUrlConnectionHttpClientBuilder).socketTimeout(socketTimeoutCaptor.capture());
+ Mockito.verify(spyUrlConnectionHttpClientBuilder)
+ .connectionTimeout(connectionTimeoutCaptor.capture());
+
+ Duration capturedSocketTimeout = socketTimeoutCaptor.getValue();
+ Duration capturedConnectionTimeout = connectionTimeoutCaptor.getValue();
+
+ Assert.assertEquals(
+ "The configured socket timeout should be 90 ms", 90, capturedSocketTimeout.toMillis());
+ Assert.assertEquals(
+ "The configured connection timeout should be 80 ms",
+ 80,
+ capturedConnectionTimeout.toMillis());
+ }
+
+ @Test
+ public void testUrlConnectionConnectionTimeoutConfiguration() {
Review Comment:
why do we need this test? isn't it a subset of the test above?
--
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 #6746: AWS: Load HttpClientBuilder dynamically to avoid runtime deps of both urlconnection and apache client
Posted by "JonasJ-ap (via GitHub)" <gi...@apache.org>.
JonasJ-ap commented on code in PR #6746:
URL: https://github.com/apache/iceberg/pull/6746#discussion_r1096965051
##########
aws/src/test/java/org/apache/iceberg/aws/TestHttpClientConfigurations.java:
##########
@@ -0,0 +1,404 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.aws;
+
+import java.time.Duration;
+import java.util.Map;
+import org.apache.iceberg.relocated.com.google.common.collect.Maps;
+import org.junit.Assert;
+import org.junit.Test;
+import org.mockito.ArgumentCaptor;
+import org.mockito.Mockito;
+import software.amazon.awssdk.http.apache.ApacheHttpClient;
+import software.amazon.awssdk.http.urlconnection.UrlConnectionHttpClient;
+
+public class TestHttpClientConfigurations {
+ @Test
+ public void testUrlConnectionConnectionSocketTimeoutConfiguration() {
+ Map<String, String> properties = Maps.newHashMap();
+ properties.put(AwsProperties.HTTP_CLIENT_URLCONNECTION_SOCKET_TIMEOUT_MS, "90");
+ properties.put(AwsProperties.HTTP_CLIENT_URLCONNECTION_CONNECTION_TIMEOUT_MS, "80");
+ AwsProperties awsProperties = new AwsProperties(properties);
+ UrlConnectionHttpClientConfigurations urlConnectionHttpClientConfigurations =
+ new UrlConnectionHttpClientConfigurations();
+ urlConnectionHttpClientConfigurations.initialize(
+ awsProperties.urlConnectionHttpClientProperties());
+ UrlConnectionHttpClient.Builder urlConnectionHttpClientBuilder =
+ UrlConnectionHttpClient.builder();
+ UrlConnectionHttpClient.Builder spyUrlConnectionHttpClientBuilder =
+ Mockito.spy(urlConnectionHttpClientBuilder);
+ ArgumentCaptor<Duration> socketTimeoutCaptor = ArgumentCaptor.forClass(Duration.class);
+ ArgumentCaptor<Duration> connectionTimeoutCaptor = ArgumentCaptor.forClass(Duration.class);
+
+ urlConnectionHttpClientConfigurations.configureUrlConnectionHttpClientBuilder(
+ spyUrlConnectionHttpClientBuilder);
+ Mockito.verify(spyUrlConnectionHttpClientBuilder).socketTimeout(socketTimeoutCaptor.capture());
+ Mockito.verify(spyUrlConnectionHttpClientBuilder)
+ .connectionTimeout(connectionTimeoutCaptor.capture());
+
+ Duration capturedSocketTimeout = socketTimeoutCaptor.getValue();
+ Duration capturedConnectionTimeout = connectionTimeoutCaptor.getValue();
+
+ Assert.assertEquals(
+ "The configured socket timeout should be 90 ms", 90, capturedSocketTimeout.toMillis());
+ Assert.assertEquals(
+ "The configured connection timeout should be 80 ms",
+ 80,
+ capturedConnectionTimeout.toMillis());
+ }
+
+ @Test
+ public void testUrlConnectionConnectionTimeoutConfiguration() {
Review Comment:
I think the purpose is to verify that the default behavior is to not configure `socketTimeout` if corresponding property is not set:
```
Mockito.verify(spyUrlConnectionHttpClientBuilder, Mockito.never())
.socketTimeout(socketTimeoutCaptor.capture());
```
I agree that these tests are redundant and we also need to test the default behavior for other configs. I plan to refactor them to have two tests testing default behavior of all configs for `UrlConnectionHttpClient` and `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