Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Integrate TimeSource into the orchestrator and middleware (breaking change warning is spurious) #2728

Merged
merged 9 commits into from
May 30, 2023

Conversation

rcoh
Copy link
Collaborator

@rcoh rcoh commented May 25, 2023

Motivation and Context

This adds TimeSource in SDK and service configs so we can effectively control time when executing requests with the SDK at the client level.

note: the breaking change is in a trait implementation of a struct we're going to delete, not a real breaking change

Description

  • Add SharedTimeSource and use it in SdkConfig / aws-config / ::Config
  • Wire up the signer and other uses of SystemTime::now() that I could find
  • track down broken tests

Testing

  • various unit tests that all still pass

Checklist

  • I have updated CHANGELOG.next.toml if I made changes to the smithy-rs codegen or runtime crates
  • I have updated CHANGELOG.next.toml if I made changes to the AWS SDK, generated SDK code, or SDK runtime crates

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

@rcoh rcoh force-pushed the timesource-integration branch 2 times, most recently from d0a8f56 to c976824 Compare May 25, 2023 16:20
@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@rcoh rcoh force-pushed the timesource-integration branch 3 times, most recently from 9cd150e to 5a4ab98 Compare May 25, 2023 18:34
@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@rcoh rcoh marked this pull request as ready for review May 25, 2023 20:51
@rcoh rcoh requested review from a team as code owners May 25, 2023 20:51
@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

Copy link
Collaborator

@jdisanti jdisanti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

#[derive(Debug, Clone)]
// TODO(breakingChangeWindow): Delete this struct
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also add #[deprecated]? You can also probably just remove it now...

fn set_request_time(&mut self, request_time: RequestTime) {
self.put::<RequestTime>(request_time);
fn set_request_time(&mut self, request_time: impl TimeSource + 'static) {
self.put::<SharedTimeSource>(SharedTimeSource::new(request_time));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this end up being a Box<Arc<...>> under the hood due to the ConfigBag implementation?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah (but so does every thing we put in the ConfigBag, unfortunately)

there is a little trickery we could do to avoid that double-allocation with the new load/store traits because they can offer an enum that exposes the internals of the object so the config bag can use the smart pointer directly. not sure if it's worth it

@@ -70,7 +72,7 @@ class CustomizableOperationTestHelpers(runtimeConfig: RuntimeConfig) :
pub fn request_time_for_tests(mut self, request_time: ::std::time::SystemTime) -> Self {
use #{ConfigBagAccessors};
let interceptor = #{TestParamsSetterInterceptor}::new(move |_: &mut #{BeforeTransmitInterceptorContextMut}<'_>, cfg: &mut #{ConfigBag}| {
cfg.set_request_time(#{RequestTime}::new(request_time));
cfg.set_request_time(#{StaticTimeSource}::new(request_time));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is StaticTimeSource available in codegenScope?

@rcoh rcoh added the breaking-change This will require a breaking change label May 30, 2023
@rcoh rcoh changed the title Integrate TimeSource into the orchestrator and middleware Integrate TimeSource into the orchestrator and middleware (breaking change warning is spurious) May 30, 2023
@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@rcoh rcoh enabled auto-merge May 30, 2023 15:29
@rcoh rcoh added this pull request to the merge queue May 30, 2023
@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

Merged via the queue into main with commit b30b063 May 30, 2023
@rcoh rcoh deleted the timesource-integration branch May 30, 2023 16:15
@rcoh rcoh mentioned this pull request May 31, 2023
2 tasks
jdisanti pushed a commit that referenced this pull request Jun 16, 2023
…mode (#2762)

## Motivation and Context
Service config structs now only contain a
`aws_smithy_types::config_bag::FrozenLayer` in the orchestrator mode (no
changes for the middleware mode).

## Description
This PR reduces the individual fields of service configs to contain just
`FrozenLayer`. This makes service configs work more seamlessly with
runtime plugins in the orchestrator mode.

Note that service config _builder_ s still contain individual fields.
We're planning to make them just contain
`aws_smithy_types::config_bag::Layer`. To do that, though, we need to
make `Layer` cloneable and that will be handled in a separate PR. For
builders, the only change you will in the PR is that their `build`
method will put fields into a `Layer`, freeze it, and pass it to service
configs.

This PR is marked as a breaking change because it's based on [another
PR](#2728) that's also breaking
change.

## Testing
- [x] Passed tests in CI

----

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

---------

Co-authored-by: Yuki Saito <awsaito@amazon.com>
Co-authored-by: Zelda Hessler <zhessler@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change This will require a breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants