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

create an IdentityCache by default when on latest BehaviorVersion #3578

Merged
merged 5 commits into from
Apr 26, 2024

Conversation

aajtodd
Copy link
Contributor

@aajtodd aajtodd commented Apr 11, 2024

Motivation and Context

Original issue: #3427
RFC: #3544

Description

Make ConfigLoader create an IdentityCache by default when on the latest BehaviorVersion. This will allow SDK clients created from SdkConfig to share a cache by default instead of creating one per/client.

Checklist

  • 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.

Copy link

A new generated diff is ready to view.

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

A new doc preview is ready to view.

Base automatically changed from jdisanti-bmv-stall-uploads to behavior-version-2024-03-28 April 24, 2024 19:20
@aajtodd aajtodd marked this pull request as ready for review April 25, 2024 13:45
@aajtodd aajtodd requested review from a team as code owners April 25, 2024 13:45
@aajtodd aajtodd removed the blocked label Apr 25, 2024
Copy link

A new generated diff is ready to view.

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

A new doc preview is ready to view.

@@ -238,14 +239,14 @@ mod loader {
use crate::provider_config::ProviderConfig;

#[derive(Default, Debug)]
enum CredentialsProviderOption {
/// No provider was set by the user. We can set up the default credentials provider chain.
enum TriStateOption<T> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We used to have a TriState in aws-smithy-types. You could consider resurrecting that if you want to, but doesn't matter to me either way since this is private.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we have need for another tri-state then we can move it but for now I think I'd keep it private.

/// .await;
/// # }
/// ```
pub fn no_identity_cache(mut self) -> Self {
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 behave differently from loader.identity_cache(IdentityCache::no_cache())?

Copy link
Contributor Author

@aajtodd aajtodd Apr 25, 2024

Choose a reason for hiding this comment

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

Yes. IdentityCache::no_cache() is still an explicit cache being set whereas no_identity_cache() leaves it as "explicitly unset" so as to not enable the default cache. Open to bikeshedding on naming and how to best avoid confusion here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Discussed offline and decided to remove this API for now as it can always be added later and users can still control this by setting a different cache per/client.

Copy link

A new generated diff is ready to view.

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

A new doc preview is ready to view.

Copy link

A new generated diff is ready to view.

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

A new doc preview is ready to view.

@aajtodd aajtodd merged commit 3b87e39 into behavior-version-2024-03-28 Apr 26, 2024
44 checks passed
@aajtodd aajtodd deleted the atodd/bmv-identity-cache branch April 26, 2024 18:45
@aajtodd aajtodd mentioned this pull request Apr 30, 2024
2 tasks
github-merge-queue bot pushed a commit that referenced this pull request May 2, 2024
## Motivation and Context
This PR is the combination of two previously reviewed PRs that both
enable new behaviors behind a new Behavior Major Version (BMV):
* #3527
* #3578

## Description
* Enables stalled stream protection by default on latest behavior
version.
* Enables creation of a default identity cache.

## Testing
All testing done on prior PRs. See for details.

## Checklist
<!--- If a checkbox below is not applicable, then please DELETE it
rather than leaving it unchecked -->
- [x] I have updated `CHANGELOG.next.toml` if I made changes to the
smithy-rs codegen or runtime crates
- [x] 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._

---------

Co-authored-by: John DiSanti <jdisanti@amazon.com>
Co-authored-by: ysaito1001 <awsaito@amazon.com>
ogghead pushed a commit to ogghead/smithy-rs that referenced this pull request May 5, 2024
This PR is the combination of two previously reviewed PRs that both
enable new behaviors behind a new Behavior Major Version (BMV):
* smithy-lang#3527
* smithy-lang#3578

* Enables stalled stream protection by default on latest behavior
version.
* Enables creation of a default identity cache.

All testing done on prior PRs. See for details.

<!--- If a checkbox below is not applicable, then please DELETE it
rather than leaving it unchecked -->
- [x] I have updated `CHANGELOG.next.toml` if I made changes to the
smithy-rs codegen or runtime crates
- [x] 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._

---------

Co-authored-by: John DiSanti <jdisanti@amazon.com>
Co-authored-by: ysaito1001 <awsaito@amazon.com>
aws-sdk-rust-ci pushed a commit to awslabs/aws-sdk-rust that referenced this pull request May 8, 2024
## Motivation and Context
This PR is the combination of two previously reviewed PRs that both
enable new behaviors behind a new Behavior Major Version (BMV):
* smithy-lang/smithy-rs#3527
* smithy-lang/smithy-rs#3578

## Description
* Enables stalled stream protection by default on latest behavior
version.
* Enables creation of a default identity cache.

## Testing
All testing done on prior PRs. See for details.

## Checklist
<!--- If a checkbox below is not applicable, then please DELETE it
rather than leaving it unchecked -->
- [x] I have updated `CHANGELOG.next.toml` if I made changes to the
smithy-rs codegen or runtime crates
- [x] 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._

---------

Co-authored-by: John DiSanti <jdisanti@amazon.com>
Co-authored-by: ysaito1001 <awsaito@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants