Endpoint Discovery for DynamoDb #853

spfink · 2018-11-20T17:10:03Z

Supports the endpoint discovery spec for DynamoDb.

varunnvs92 · 2018-11-20T20:39:48Z

...test/resources/software/amazon/awssdk/codegen/poet/client/test-endpoint-discovery-async.java

+            URI cachedEndpoint = endpointDiscoveryCache.get(key,
+                    testDiscoveryIdentifiersRequiredRequest.endpointDiscoveryRequest(),
+                    clientConfiguration.option(SdkClientOption.ENDPOINT));
+            clientConfiguration.copy(o -> o.option(SdkClientOption.ENDPOINT, cachedEndpoint));


This doesn't change the original configuration. Also we don't want to update the client endpoint right as the new computed endpoint is only valid for that operation.

For endpoint trait, I am passing the hostPrefix to CientExecutionParams and modifying the endpoint in BaseClientHandler. But this is causing issues with path as its being modified multiple times. I am thinking of passing the hostprefix to marshallers and resolve the endpoint there before creating the HttpFullRequest.

Ah, yes, was on a different version when I was doing this. Will update.

shorea · 2018-12-28T21:20:15Z

...test/resources/software/amazon/awssdk/codegen/poet/client/test-endpoint-discovery-async.java

+            HttpResponseHandler<AwsServiceException> errorResponseHandler = createErrorResponseHandler(protocolFactory,
+                    operationMetadata);
+
+            String key = clientConfiguration.option(AwsClientOption.CREDENTIALS_PROVIDER).resolveCredentials().accessKeyId();


Would this not take into account request level credentials?

Have same question

shorea · 2018-12-28T21:21:49Z

...test/resources/software/amazon/awssdk/codegen/poet/client/test-endpoint-discovery-async.java

+                    operationMetadata);
+
+            String key = clientConfiguration.option(AwsClientOption.CREDENTIALS_PROVIDER).resolveCredentials().accessKeyId();
+            URI cachedEndpoint = endpointDiscoveryCache.get(key,


This cachedEndpoint doesn't appear to be used. Also for async this seems like it would be kind of blocking, at least for the first call.

It would only be blocking if endpoint discovery is set as required. Otherwise we use the default endpoint. Is that correct?

shorea · 2018-12-28T21:24:28Z

core/aws-core/pom.xml

@@ -33,6 +33,11 @@
            <artifactId>auth</artifactId>
            <version>${awsjavasdk.version}</version>
        </dependency>
+        <dependency>
+            <groupId>software.amazon.awssdk</groupId>
+            <artifactId>profiles</artifactId>


Why do we need this dependency?

You can enable endpoint discovery through the profile file so I have a provider for it that requires the profiles module.

shorea · 2018-12-28T21:25:37Z

...rc/main/java/software/amazon/awssdk/awscore/endpointdiscovery/EndpointDiscoveryEndpoint.java

+import software.amazon.awssdk.annotations.SdkInternalApi;
+
+@SdkInternalApi
+public class EndpointDiscoveryEndpoint {


shorea · 2018-12-28T21:26:12Z

...rc/main/java/software/amazon/awssdk/awscore/endpointdiscovery/EndpointDiscoveryEndpoint.java

+        return builder().endpoint(endpoint).expirationTime(expirationTime);
+    }
+
+    public interface Builder {


Should these extend any of that SdkBuilder copyable builder stuff?

shorea · 2018-12-28T21:37:13Z

...ain/java/software/amazon/awssdk/awscore/endpointdiscovery/EndpointDiscoveryRefreshCache.java

+            }
+        }
+
+        if (endpoint.expirationTime().isBefore(Instant.now())) {


Does this mean it's already expired? If so are we returning the expired endpoint to the customer? Should we attempt to refresh it before it expires?

Expired endpoints can be used until a 421 status code or InvalidEndpointException is thrown.

Once an endpoint is expired, we extend the expiration to 60 seconds and immediately attempt a refresh. We extend the expiration 60 seconds so that we aren't refreshing every time.

Hmm gotcha. So expired means will become invalid soon?

shorea · 2018-12-28T21:37:57Z

...src/main/java/software/amazon/awssdk/awscore/endpointdiscovery/EndpointDiscoveryRequest.java

+        return operationName;
+    }
+
+    public Optional<Map<String, String>> identifiers() {


Should we copy and make this map unmodifiable in constructor?

We certainly could but not sure if we need to given this is an internal use only class.

shorea · 2018-12-28T21:39:23Z

...va/software/amazon/awssdk/awscore/endpointdiscovery/providers/EndpointDiscoveryProvider.java

+@SdkInternalApi
+public interface EndpointDiscoveryProvider {
+
+    boolean resolveEndpointDiscovery();


Can you add some docs here? I'm not sure what boolean is supposed to represent, I guess if endpoint discovery is enabled?

shorea · 2018-12-28T21:42:28Z

...ware/amazon/awssdk/codegen/poet/endpointdiscovery/EndpointDiscoveryCacheLoaderGenerator.java

+import software.amazon.awssdk.codegen.poet.PoetExtensions;
+import software.amazon.awssdk.codegen.poet.PoetUtils;
+
+public class EndpointDiscoveryCacheLoaderGenerator implements ClassSpec {


Can you add some code gen tests for this thing?

shorea · 2018-12-28T21:43:23Z

codegen/src/main/java/software/amazon/awssdk/codegen/poet/model/AwsServiceModel.java

@@ -366,6 +372,15 @@ private MethodSpec getValueForField() {
        return methodBuilder.build();
    }

+    private MethodSpec endpointDiscoveryRequest() {


Do we need to put this on the service model? Seems like an odd place for it.

Any alternative suggestions?

If not on the generated models for the requests, we'd have to either generate a separate class per request or generate a list of which fields need to be pulled out for the identifiers for each request.

This method is only called in the client. So what do you think about generating in the client method impl?

Code in cilent will change from

endpointDiscoveryCache.get(key, listBackupsRequest.endpointDiscoveryRequest(), clientConfiguration.option(SdkClientOption.ENDPOINT));

to

endpointDiscoveryCache.get(key, EndpointDiscoveryRequest.builder().required(false).build(), clientConfiguration.option(SdkClientOption.ENDPOINT));

codecov-io · 2019-01-04T00:33:57Z

Codecov Report

Merging #853 into master will decrease coverage by 0.02%.
The diff coverage is 52.67%.

@@             Coverage Diff              @@
##             master     #853      +/-   ##
============================================
- Coverage     55.61%   55.58%   -0.03%     
- Complexity     4510     4566      +56     
============================================
  Files           799      810      +11     
  Lines         27487    27813     +326     
  Branches       2222     2241      +19     
============================================
+ Hits          15286    15461     +175     
- Misses        11488    11638     +150     
- Partials        713      714       +1

Impacted Files	Coverage Δ	Complexity Δ
...oviders/DefaultEndpointDiscoveryProviderChain.java	`0% <0%> (ø)`	`0 <0> (?)`
...dpointdiscovery/EndpointDiscoveryRefreshCache.java	`0% <0%> (ø)`	`0 <0> (?)`
...very/providers/EndpointDiscoveryProviderChain.java	`0% <0%> (ø)`	`0 <0> (?)`
...egen/emitters/tasks/AsyncClientGeneratorTasks.java	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
...degen/emitters/tasks/SyncClientGeneratorTasks.java	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
...ndpointdiscovery/EndpointDiscoveryCacheLoader.java	`0% <0%> (ø)`	`0 <0> (?)`
...e/endpointdiscovery/EndpointDiscoveryEndpoint.java	`0% <0%> (ø)`	`0 <0> (?)`
...ers/SystemPropertiesEndpointDiscoveryProvider.java	`0% <0%> (ø)`	`0 <0> (?)`
...ry/providers/ProfileEndpointDiscoveryProvider.java	`0% <0%> (ø)`	`0 <0> (?)`
...degen/poet/builder/BaseClientBuilderInterface.java	`83.33% <0%> (-13.89%)`	`6 <0> (ø)`
... and 50 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a58b325...78cc403. Read the comment docs.

varunnvs92 · 2019-01-07T18:10:49Z

codegen/src/main/java/software/amazon/awssdk/codegen/IntermediateModelBuilder.java

+
+        for (OperationModel o : operations.values()) {
+            if (o.isEndpointOperation()) {
+                endpointOperation = o;


Nit: we can break after finding the operation.

varunnvs92 · 2019-01-07T22:39:38Z

codegen/src/main/java/software/amazon/awssdk/codegen/poet/model/AwsServiceModel.java

@@ -366,6 +372,15 @@ private MethodSpec getValueForField() {
        return methodBuilder.build();
    }

+    private MethodSpec endpointDiscoveryRequest() {


This method is only called in the client. So what do you think about generating in the client method impl?

Code in cilent will change from

endpointDiscoveryCache.get(key, listBackupsRequest.endpointDiscoveryRequest(), clientConfiguration.option(SdkClientOption.ENDPOINT));

to

endpointDiscoveryCache.get(key, EndpointDiscoveryRequest.builder().required(false).build(), clientConfiguration.option(SdkClientOption.ENDPOINT));

varunnvs92 · 2019-01-07T22:50:52Z

...test/resources/software/amazon/awssdk/codegen/poet/client/test-endpoint-discovery-async.java

+                    operationMetadata);
+
+            String key = clientConfiguration.option(AwsClientOption.CREDENTIALS_PROVIDER).resolveCredentials().accessKeyId();
+            URI cachedEndpoint = endpointDiscoveryCache.get(key,


It would only be blocking if endpoint discovery is set as required. Otherwise we use the default endpoint. Is that correct?

varunnvs92 · 2019-01-07T22:51:17Z

...test/resources/software/amazon/awssdk/codegen/poet/client/test-endpoint-discovery-async.java

+            HttpResponseHandler<AwsServiceException> errorResponseHandler = createErrorResponseHandler(protocolFactory,
+                    operationMetadata);
+
+            String key = clientConfiguration.option(AwsClientOption.CREDENTIALS_PROVIDER).resolveCredentials().accessKeyId();


Have same question

varunnvs92 · 2019-01-07T22:54:57Z

...c/main/java/software/amazon/awssdk/core/endpointdiscovery/EndpointDiscoveryRefreshCache.java

+        if (endpoint.expirationTime().isBefore(Instant.now())) {
+            cache.put(key, endpoint.toBuilder().expirationTime(Instant.now().plusSeconds(60)).build());
+            String finalKey = key;
+            discoverEndpoint(null).thenApply(v -> cache.put(finalKey, v));


Is passing null request to discoverEndpoint method right?

Changed to the request object.

varunnvs92 · 2019-01-07T22:55:46Z

...re/src/main/java/software/amazon/awssdk/core/endpointdiscovery/EndpointDiscoveryRequest.java

+        return new BuilderImpl(this);
+    }
+
+    public interface Builder extends CopyableBuilder<Builder, EndpointDiscoveryRequest> {


spfink · 2019-01-11T00:36:52Z

@varunnvs92, can't reply to some of your comments for some reason.

In the async client, it would only be blocking if endpoint discovery is required. Otherwise, it will be non-blocking.

varunnvs92

LGTM

…e6bbd825 Pull request: release <- staging/fc650e8a-ed4f-453a-b1b1-4619e6bbd825

varunnvs92 reviewed Nov 20, 2018

View reviewed changes

shorea reviewed Dec 28, 2018

View reviewed changes

varunnvs92 reviewed Jan 7, 2019

View reviewed changes

varunnvs92 approved these changes Jan 11, 2019

View reviewed changes

CRR support for DynamoDB

78cc403

spfink force-pushed the finks/endpoint-discovery branch from 17d093b to 78cc403 Compare January 17, 2019 03:57

Merge branch 'master' into finks/endpoint-discovery

4d87971

spfink merged commit 938a73b into master Jan 17, 2019

spfink deleted the finks/endpoint-discovery branch March 17, 2020 23:41

aws-sdk-java-automation pushed a commit that referenced this pull request May 21, 2020

Merge pull request #853 from aws/staging/fc650e8a-ed4f-453a-b1b1-4619…

a92f967

…e6bbd825 Pull request: release <- staging/fc650e8a-ed4f-453a-b1b1-4619e6bbd825

Endpoint Discovery for DynamoDb #853

Endpoint Discovery for DynamoDb #853

Uh oh!

Conversation

spfink commented Nov 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Jan 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spfink commented Jan 11, 2019

Uh oh!

varunnvs92 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

spfink commented Nov 20, 2018 •

edited

Loading

codecov-io commented Jan 4, 2019 •

edited

Loading