Implement waiters #3595

jdisanti · 2024-04-19T19:46:00Z

This PR implements waiters according to the Smithy waiters spec.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

github-actions · 2024-04-23T21:38:38Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
Client Test (ignoring whitespace)
No codegen difference in the Server Test
Server Test Python (ignoring whitespace)
No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

jdisanti · 2024-04-23T22:45:18Z

rust-runtime/aws-smithy-runtime-api/src/client/runtime_components.rs

    /// Wraps `v` in tracking associated with this builder
    fn tracked<T>(&self, v: Option<T>) -> Option<Tracked<T>> {
        v.map(|v| Tracked::new(self.builder_name, v))
    }
 }

+// TODO(waiters): Decide if this is the right approach for extracting sleep_impl/time_source


I think this is the only big outstanding question on this PR. How exactly do we want to access components outside of the orchestrator prior to validation?

Whats the alternative? What are your concerns with this approach?

I can think of a couple of alternatives:

Add getter functions to RuntimeComponentsBuilder, or

Create a OptionalRuntimeComponents struct that is analog to RuntimeComponents, but where creation doesn't do validation, and all the components in it are wrapped in Option. The RuntimeComponentsBuilder would have a build_optional function to create this.

The current approach is limited to time/sleep. In the future, some other non-time related component may be needed outside of the orchestrator. Given that, I think these alternatives would be better, but then it's just a matter of picking between them.

I don't think what you have is a one way door so I'm not too concerned.

aajtodd

Overall looks great! Few questions and some discussion.

...oftware/amazon/smithy/rust/codegen/client/smithy/generators/client/FluentBuilderGenerator.kt

aws/sdk/integration-tests/ec2/tests/waiters.rs

.../software/amazon/smithy/rust/codegen/client/smithy/generators/error/ServiceErrorGenerator.kt

...in/software/amazon/smithy/rust/codegen/client/smithy/generators/waiters/WaitableGenerator.kt

aajtodd · 2024-04-24T13:40:49Z

rust-runtime/aws-smithy-runtime-api/src/client/waiters.rs

+    /// time being exceeded, or some other failure occurring.
+    #[derive(Debug)]
+    #[non_exhaustive]
+    pub enum WaiterError<O, E> {


rust-runtime/aws-smithy-runtime/src/client/waiters.rs

aajtodd · 2024-04-24T13:47:52Z

rust-runtime/aws-smithy-runtime-api/src/client/runtime_components.rs

    /// Wraps `v` in tracking associated with this builder
    fn tracked<T>(&self, v: Option<T>) -> Option<Tracked<T>> {
        v.map(|v| Tracked::new(self.builder_name, v))
    }
 }

+// TODO(waiters): Decide if this is the right approach for extracting sleep_impl/time_source


Whats the alternative? What are your concerns with this approach?

github-actions · 2024-04-24T23:34:03Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
Client Test (ignoring whitespace)
No codegen difference in the Server Test
Server Test Python (ignoring whitespace)
No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

ysaito1001

Still going through the PR, but looks great so far. One clarifying question. .customize and .wait_XXX on a fluent builder cannot be used together, i.e. the following is not supported?

client
    .operation()
    .customize()
    .config_override(...)
    .wait_until_XXX
    ...

ysaito1001

Excellent work! In codegen diff, I see the doc on a waiter's fluent builder look like

impl ImageExistsFluentBuilder {
    /// Creates a new `DescribeImages`.
    pub(crate) fn new(handle: ::std::sync::Arc<crate::client::Handle>) -> Self {
        Self {
            handle,
            inner: ::std::default::Default::default(),
        }
    }

It says /// Creates a new DescribeImages but not /// Creates a new ImageExistsFluentBuilder or /// Creates a new ImageExists. Is that expected?

jdisanti · 2024-04-25T17:38:11Z

Still going through the PR, but looks great so far. One clarifying question. .customize and .wait_XXX on a fluent builder cannot be used together, i.e. the following is not supported?

I opted to not support this yet without better understanding the use-case for it. It won't be hard to add if a good reason comes up.

jdisanti · 2024-04-25T17:41:43Z

It says /// Creates a new DescribeImages but not /// Creates a new ImageExistsFluentBuilder or /// Creates a new ImageExists. Is that expected?

No, this is a bug. Fixed in 8a3d012

github-actions · 2024-04-25T17:59:14Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
Client Test (ignoring whitespace)
No codegen difference in the Server Test
Server Test Python (ignoring whitespace)
No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

github-actions · 2024-04-25T18:47:32Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
Client Test (ignoring whitespace)
No codegen difference in the Server Test
Server Test Python (ignoring whitespace)
No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

jdisanti added the needs-sdk-review label Apr 19, 2024

Implement waiters

15ef73d

jdisanti force-pushed the jdisanti-waiters branch from 6c256e1 to 15ef73d Compare April 19, 2024 19:59

jdisanti added 6 commits April 19, 2024 17:40

Attempt to fix enum detection in the jmespath traverser

b2dd998

Merge remote-tracking branch 'origin/main' into jdisanti-waiters

6f8bceb

Fix NPE when generating flattens of multi-select lists

7a99b75

Merge remote-tracking branch 'origin/main' into jdisanti-waiters

666928a

Fix unintentional removal of error conversions

2dd955d

Fix feature gate issue in aws-smithy-runtime

e4d359f

smithy-lang deleted a comment from github-actions bot Apr 23, 2024

jdisanti commented Apr 23, 2024

View reviewed changes

jdisanti marked this pull request as ready for review April 23, 2024 22:45

jdisanti requested review from a team as code owners April 23, 2024 22:45

aajtodd reviewed Apr 24, 2024

View reviewed changes

jdisanti added 3 commits April 24, 2024 15:53

Rename wait_for to wait_until

853a4dc

Incorporate other feedback

637b4b6

Add correlation ID to waiter orchestration

e1e103f

aajtodd approved these changes Apr 25, 2024

View reviewed changes

ysaito1001 reviewed Apr 25, 2024

View reviewed changes

ysaito1001 approved these changes Apr 25, 2024

View reviewed changes

jdisanti mentioned this pull request Apr 25, 2024

Fix a timing issue in the waiters retry delay calculation smithy-lang/smithy#2259

Merged

Minor fixes

8a3d012

Fix doc typo

2e2030d

jdisanti added this pull request to the merge queue Apr 25, 2024

Merged via the queue into main with commit cfb97ed Apr 25, 2024
44 checks passed

jdisanti deleted the jdisanti-waiters branch April 25, 2024 19:57

Velfi removed the needs-sdk-review label May 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement waiters #3595

Implement waiters #3595

jdisanti commented Apr 19, 2024 •

edited

github-actions bot commented Apr 23, 2024

jdisanti Apr 23, 2024

aajtodd Apr 24, 2024

jdisanti Apr 24, 2024 •

edited

aajtodd Apr 24, 2024

aajtodd left a comment

aajtodd Apr 24, 2024

aajtodd Apr 24, 2024

github-actions bot commented Apr 24, 2024

ysaito1001 left a comment •

edited

ysaito1001 left a comment

jdisanti commented Apr 25, 2024

jdisanti commented Apr 25, 2024

github-actions bot commented Apr 25, 2024

github-actions bot commented Apr 25, 2024

Implement waiters #3595

Implement waiters #3595

Conversation

jdisanti commented Apr 19, 2024 • edited

github-actions bot commented Apr 23, 2024

jdisanti Apr 23, 2024

Choose a reason for hiding this comment

aajtodd Apr 24, 2024

Choose a reason for hiding this comment

jdisanti Apr 24, 2024 • edited

Choose a reason for hiding this comment

aajtodd Apr 24, 2024

Choose a reason for hiding this comment

aajtodd left a comment

Choose a reason for hiding this comment

aajtodd Apr 24, 2024

Choose a reason for hiding this comment

aajtodd Apr 24, 2024

Choose a reason for hiding this comment

github-actions bot commented Apr 24, 2024

ysaito1001 left a comment • edited

Choose a reason for hiding this comment

ysaito1001 left a comment

Choose a reason for hiding this comment

jdisanti commented Apr 25, 2024

jdisanti commented Apr 25, 2024

github-actions bot commented Apr 25, 2024

github-actions bot commented Apr 25, 2024

jdisanti commented Apr 19, 2024 •

edited

jdisanti Apr 24, 2024 •

edited

ysaito1001 left a comment •

edited