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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
