mosche commented on a change in pull request #16760:
URL: https://github.com/apache/beam/pull/16760#discussion_r808046772



##########
File path: 
sdks/java/io/amazon-web-services2/src/main/java/org/apache/beam/sdk/io/aws2/options/AwsOptions.java
##########
@@ -17,76 +17,118 @@
  */
 package org.apache.beam.sdk.io.aws2.options;
 
+import java.net.URI;
 import org.apache.beam.sdk.annotations.Experimental;
 import org.apache.beam.sdk.annotations.Experimental.Kind;
+import org.apache.beam.sdk.io.aws2.common.ClientBuilderFactory;
+import org.apache.beam.sdk.io.aws2.common.HttpClientConfiguration;
 import org.apache.beam.sdk.options.Default;
 import org.apache.beam.sdk.options.DefaultValueFactory;
 import org.apache.beam.sdk.options.Description;
 import org.apache.beam.sdk.options.PipelineOptions;
 import org.checkerframework.checker.nullness.qual.Nullable;
 import software.amazon.awssdk.auth.credentials.AwsCredentialsProvider;
+import software.amazon.awssdk.auth.credentials.ContainerCredentialsProvider;
 import software.amazon.awssdk.auth.credentials.DefaultCredentialsProvider;
+import 
software.amazon.awssdk.auth.credentials.EnvironmentVariableCredentialsProvider;
+import software.amazon.awssdk.auth.credentials.ProfileCredentialsProvider;
+import software.amazon.awssdk.auth.credentials.StaticCredentialsProvider;
+import 
software.amazon.awssdk.auth.credentials.SystemPropertyCredentialsProvider;
 import software.amazon.awssdk.core.exception.SdkClientException;
 import software.amazon.awssdk.http.apache.ProxyConfiguration;
+import software.amazon.awssdk.regions.Region;
 import software.amazon.awssdk.regions.providers.DefaultAwsRegionProviderChain;
-import software.amazon.awssdk.utils.AttributeMap;
+import 
software.amazon.awssdk.services.sts.auth.StsAssumeRoleCredentialsProvider;
 
 /**
  * Options used to configure Amazon Web Services specific options such as 
credentials and region.
  */
 @Experimental(Kind.SOURCE_SINK)
 public interface AwsOptions extends PipelineOptions {
 
-  /** AWS region used by the AWS client. */
-  @Description("AWS region used by the AWS client")
+  /** Region used to configure AWS service clients. */
+  @Description("Region used by AWS service clients")
   @Default.InstanceFactory(AwsRegionFactory.class)
-  String getAwsRegion();
+  Region getAwsRegion();

Review comment:
       This is a breaking change to improve type safety. Seems low risk as 
setting options is typically done via args

##########
File path: 
sdks/java/io/amazon-web-services2/src/main/java/org/apache/beam/sdk/io/aws2/options/AwsOptions.java
##########
@@ -17,76 +17,118 @@
  */
 package org.apache.beam.sdk.io.aws2.options;
 
+import java.net.URI;
 import org.apache.beam.sdk.annotations.Experimental;
 import org.apache.beam.sdk.annotations.Experimental.Kind;
+import org.apache.beam.sdk.io.aws2.common.ClientBuilderFactory;
+import org.apache.beam.sdk.io.aws2.common.HttpClientConfiguration;
 import org.apache.beam.sdk.options.Default;
 import org.apache.beam.sdk.options.DefaultValueFactory;
 import org.apache.beam.sdk.options.Description;
 import org.apache.beam.sdk.options.PipelineOptions;
 import org.checkerframework.checker.nullness.qual.Nullable;
 import software.amazon.awssdk.auth.credentials.AwsCredentialsProvider;
+import software.amazon.awssdk.auth.credentials.ContainerCredentialsProvider;
 import software.amazon.awssdk.auth.credentials.DefaultCredentialsProvider;
+import 
software.amazon.awssdk.auth.credentials.EnvironmentVariableCredentialsProvider;
+import software.amazon.awssdk.auth.credentials.ProfileCredentialsProvider;
+import software.amazon.awssdk.auth.credentials.StaticCredentialsProvider;
+import 
software.amazon.awssdk.auth.credentials.SystemPropertyCredentialsProvider;
 import software.amazon.awssdk.core.exception.SdkClientException;
 import software.amazon.awssdk.http.apache.ProxyConfiguration;
+import software.amazon.awssdk.regions.Region;
 import software.amazon.awssdk.regions.providers.DefaultAwsRegionProviderChain;
-import software.amazon.awssdk.utils.AttributeMap;
+import 
software.amazon.awssdk.services.sts.auth.StsAssumeRoleCredentialsProvider;
 
 /**
  * Options used to configure Amazon Web Services specific options such as 
credentials and region.
  */
 @Experimental(Kind.SOURCE_SINK)
 public interface AwsOptions extends PipelineOptions {
 
-  /** AWS region used by the AWS client. */
-  @Description("AWS region used by the AWS client")
+  /** Region used to configure AWS service clients. */
+  @Description("Region used by AWS service clients")
   @Default.InstanceFactory(AwsRegionFactory.class)
-  String getAwsRegion();
+  Region getAwsRegion();
 
-  void setAwsRegion(String value);
+  void setAwsRegion(Region region);
 
   /** Attempt to load default region. */
-  class AwsRegionFactory implements DefaultValueFactory<@Nullable String> {
+  class AwsRegionFactory implements DefaultValueFactory<@Nullable Region> {
     @Override
     @Nullable
-    public String create(PipelineOptions options) {
+    public Region create(PipelineOptions options) {
       try {
-        return new DefaultAwsRegionProviderChain().getRegion().id();
+        return new DefaultAwsRegionProviderChain().getRegion();
       } catch (SdkClientException e) {
         return null;
       }
     }
   }
 
-  /** The AWS service endpoint used by the AWS client. */
-  @Description("AWS service endpoint used by the AWS client")
-  String getEndpoint();
+  /** Endpoint used to configure AWS service clients. */
+  @Description("Endpoint used by AWS service clients")
+  URI getEndpoint();

Review comment:
       This is a breaking change to improve type safety. Considering endpoints 
are primarily used for testing purposes, there seems to be very little risk.

##########
File path: 
sdks/java/io/amazon-web-services2/src/main/java/org/apache/beam/sdk/io/aws2/options/AwsOptions.java
##########
@@ -95,50 +137,43 @@ public AwsCredentialsProvider create(PipelineOptions 
options) {
   }
 
   /**
-   * The client configuration instance that should be used to configure AWS 
service clients. Please
-   * note that the configuration deserialization only allows one to specify 
proxy settings.
+   * {@link ProxyConfiguration} used to configure AWS service clients.
+   *
+   * <p>Note, only the options shown in the example below are supported. 
<code>username</code> and
+   * <code>password</code> are optional.
+   *
+   * <p>Example:
    *
-   * <p>For example, to specify the proxy endpoint, username and password, 
specify the following:
-   * <code>
-   * --proxyConfiguration={
+   * <pre>{@code --proxyConfiguration={
    *   "endpoint": "http://hostname:port";,
    *   "username": "username",
    *   "password": "password"
-   * }
-   * </code>
+   * }}</pre>
    */
   @Description(
-      "The proxy configuration instance that should be used to configure AWS 
service "
-          + "clients. Please note that the configuration deserialization only 
allows one to specify "
-          + "proxy settings. For example, to specify the proxy endpoint, 
username and password, "
-          + "specify the following: 
--proxyConfiguration={\"endpoint\":\"http://hostname:port\";, 
\"username\":\"username\", \"password\":\"password\"}")
+      "The proxy configuration used to configure AWS service clients. Example: 
"
+          + "--proxyConfiguration={\"endpoint\":\"http://hostname:port\";, 
\"username\":\"username\", \"password\":\"password\"}")
   ProxyConfiguration getProxyConfiguration();
 
   void setProxyConfiguration(ProxyConfiguration value);
 
   /**
-   * The client configuration instance that should be used to configure AWS 
service clients. Please
-   * note that the configuration deserialization allows aws http client 
configuration settings.
+   * {@link HttpClientConfiguration} used to configure AWS service clients.
    *
-   * <p>For example, to set different timeout for aws client service : Note 
that all the below
-   * fields are optional, so only add those configurations that need to be 
set. <code>
-   * --attributeMap={
-   *   "connectionAcquisitionTimeout":"PT1000S",
-   *   "connectionMaxIdleTime":"PT3000S",
-   *   "connectionTimeout":"PT10000S",
-   *   "socketTimeout":"PT600S",
-   *   "maxConnections":"10",
-   *   "socketTimeout":"PT5000SS"
-   * }
-   * </code>
+   * <p>Example:
    *
-   * @return
+   * <pre>{@code --httpClientConfiguration={"socketTimeout":1000, 
"maxConnections":10}}</pre>
    */
   @Description(
-      "The attribute map instance that should be used to configure AWS http 
client configuration parameters."
-          + "Mentioned parameters are the available parameters that can be 
set. All above parameters are "
-          + "optional set only those that need custom changes.")
-  AttributeMap getAttributeMap();
+      "The HTTP client configuration used to configure AWS service clients. 
Example: "
+          + 
"--httpClientConfiguration={\"socketTimeout\":1000,\"maxConnections\":10}")
+  HttpClientConfiguration getHttpClientConfiguration();
+
+  void setHttpClientConfiguration(HttpClientConfiguration value);
+
+  @Description("Factory class to configure AWS client builders")
+  @Default.Class(ClientBuilderFactory.DefaultClientBuilder.class)
+  Class<? extends ClientBuilderFactory> getClientBuilderFactory();
 
-  void setAttributeMap(AttributeMap attributeMap);

Review comment:
       Replaced, but wasn't used at all previously ...




-- 
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: github-unsubscr...@beam.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


Reply via email to