Add support for generating waiter acceptors. #2036

millems · 2020-09-14T18:39:39Z

This required the following changes:

Added a JMES Path interpreter based on the JMES Path parser from a previous commit. This interpreter is in the code generator and creates the Java instructions for executing the acceptor logic.
Added the code generator logic for specifying the acceptors.
Added a Waiters runtime that is currently shared between all service clients, used by the acceptors of all services.
Added a "MemberName" field to the SDK field metadata, so that it can be referenced by the waiter code (waiters refer to the member names from the C2J).
Added tests for the two most complex waiter classes: AutoScaling and ECS
Added support for acceptors matching supertypes of the response type.

Waiters should mostly be working now, but there are a few remaining tasks:

There's a WaitersRuntime component in sdk-core that should be copied to each of the service to remove them as a protected API.
Waiters currently only support the JMESPath syntax currently used by services. We should add support for some of the other 'easier' JMESPath syntax so that we're future-compatible with more waiters.

dagnir · 2020-09-14T23:25:37Z

codegen/src/main/java/software/amazon/awssdk/codegen/poet/waiters/BaseWaiterClassSpec.java

+                      .add(")");
+                break;
+            case "pathAny":
+                result.add("OnResponseAcceptor(")


Can this be move out of the switch? Looks like we always add except for status which doesn't use the result that's being built

Actually, the error path is different, so I can't do this.

codegen/src/main/java/software/amazon/awssdk/codegen/poet/waiters/BaseWaiterClassSpec.java

codegen/src/main/java/software/amazon/awssdk/codegen/poet/waiters/JmesPathInterpreter.java

codegen/src/test/java/software/amazon/awssdk/codegen/poet/waiters/JmesPathInterpreterTest.java

dagnir · 2020-09-14T23:55:25Z

core/sdk-core/src/main/java/software/amazon/awssdk/core/waiters/WaitersRuntime.java

+ * Contains classes used at runtime by the code generator classes for waiter acceptors generated from JMESPath expressions.
+ */
+@SdkProtectedApi
+public final class WaitersRuntime {


Can we add tests for the runtime?

That's a good idea, but I'm wondering how that will jive when we move the runtime to being copied into each of the service clients. I was hoping testing it in the context it's used (i.e. testing the generated waiters and making sure they worked, which indirectly tests the runtime) would be sufficient to side-step this problem. Do you have any ideas on how to make that work?

Do you have any ideas on how to make that work?

I think if we just accessed a WaitersRuntime class from within codegen-generated-classes-test that could work.

That makes sense 👍

dagnir

Looks like JMESPath has a suite of acceptance tests. Can we integrate them into our implementation somehow?

https://github.com/jmespath/jmespath.test

millems

For the JMESPath acceptance tests, there's two issues:

We don't support all of JMESPath, so we'd have to pick out the tests that we do support and use those, or expand to support all of JMESPath.
Those tests are written to work against JSON, and our implementation works against generated SdkPojos. To bridge the gap, we'd have to build a test harness to map the two, as well as parse and execute the acceptance tests against the SdkPojo -> JSON mapping.

I think that would add another week or more to the task. While I don't mind us doing it, it might be a bit much to do as part of this code review.

dagnir · 2020-09-15T20:14:23Z

I think that would add another week or more to the task. While I don't mind us doing it, it might be a bit much to do as part of this code review.

Yep I think doing that as a separate task is fine; I imagine the middle layer would look something like our protocol test harness. We should integrate any tests that we can to give us extra confidence :).

This required the following changes: 1. Added a JMES Path interpreter based on the JMES Path parser from a previous commit. This interpreter is in the code generator and creates the Java instructions for executing the acceptor logic. 2. Added the code generator logic for specifying the acceptors. 3. Added a Waiters runtime that is currently shared between all service clients, used by the acceptors of all services. 4. Added a "MemberName" field to the SDK field metadata, so that it can be referenced by the waiter code (waiters refer to the member names from the C2J). 5. Added tests for the two most complex waiter classes: AutoScaling and ECS 6. Added support for acceptors matching supertypes of the response type. Waiters should mostly be working now, but there are a few remaining tasks: 1. There's a WaitersRuntime component in sdk-core that should be copied to each of the service to remove them as a protected API. 2. Waiters currently only support the JMESPath syntax currently used by services. We should add support for some of the other 'easier' JMESPath syntax so that we're future-compatible with more waiters.

dagnir

LGTM! Please remember to add a task in the backlog if we don't have one for integrating the JMESPath suite

sonarqubecloud · 2020-09-15T23:14:49Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
15 Code Smells

76.4% Coverage
0.2% Duplication

The version of Java (1.8.0_252) you have used to run this analysis is deprecated and we will stop accepting it from October 2020. Please update to at least Java 11.
Read more here

…1fc79e8e0 Pull request: release <- staging/f37977e1-bd56-4fd8-bea9-c411fc79e8e0

Add waiter generator task

9b9a59a

dagnir reviewed Sep 14, 2020

View reviewed changes

millems commented Sep 15, 2020

View reviewed changes

millems force-pushed the millem/waiter-acceptors branch from 0041c0b to 16b7fb7 Compare September 15, 2020 18:37

millems force-pushed the millem/waiter-acceptors branch from 16b7fb7 to 89a1952 Compare September 15, 2020 21:58

millems force-pushed the millem/waiter-acceptors branch from 89a1952 to 5c607cb Compare September 15, 2020 22:50

dagnir approved these changes Sep 15, 2020

View reviewed changes

millems merged commit e371de4 into waiters-development Sep 15, 2020

millems deleted the millem/waiter-acceptors branch September 15, 2020 23:17

aws-sdk-java-automation added a commit that referenced this pull request May 19, 2022

Merge pull request #2036 from aws/staging/f37977e1-bd56-4fd8-bea9-c41…

96968c8

…1fc79e8e0 Pull request: release <- staging/f37977e1-bd56-4fd8-bea9-c411fc79e8e0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add support for generating waiter acceptors. #2036

Add support for generating waiter acceptors. #2036

Uh oh!

millems commented Sep 14, 2020 •

edited

Loading

Uh oh!

dagnir Sep 14, 2020

Uh oh!

millems Sep 15, 2020

Uh oh!

millems Sep 15, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dagnir Sep 14, 2020

Uh oh!

millems Sep 15, 2020

Uh oh!

dagnir Sep 15, 2020

Uh oh!

millems Sep 15, 2020 •

edited

Loading

Uh oh!

dagnir left a comment •

edited

Loading

Uh oh!

millems left a comment •

edited

Loading

Uh oh!

dagnir commented Sep 15, 2020

Uh oh!

dagnir left a comment

Uh oh!

sonarqubecloud bot commented Sep 15, 2020

Uh oh!

Uh oh!

Add support for generating waiter acceptors. #2036

Add support for generating waiter acceptors. #2036

Uh oh!

Conversation

millems commented Sep 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dagnir Sep 14, 2020

Choose a reason for hiding this comment

Uh oh!

millems Sep 15, 2020

Choose a reason for hiding this comment

Uh oh!

millems Sep 15, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dagnir Sep 14, 2020

Choose a reason for hiding this comment

Uh oh!

millems Sep 15, 2020

Choose a reason for hiding this comment

Uh oh!

dagnir Sep 15, 2020

Choose a reason for hiding this comment

Uh oh!

millems Sep 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dagnir left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

millems left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dagnir commented Sep 15, 2020

Uh oh!

dagnir left a comment

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Sep 15, 2020

Uh oh!

Uh oh!

millems commented Sep 14, 2020 •

edited

Loading

millems Sep 15, 2020 •

edited

Loading

dagnir left a comment •

edited

Loading

millems left a comment •

edited

Loading