Flexible Checksums V2 #3967
Flexible Checksums V2 #3967
Conversation
It sets default values for the ChecksumAlgorithm field (although that doesn't actually seem to be reflected in the code at the moment)
Value is not yet added to ConfigBag
Still need to implement tests from SEP
Note: it doesn't actually work because sigv4 is benig added as a dev dep but is required by one of the inlineable interceptors as a runtime dep so that still needs to be fixed.
Update codegen to point to new checksum config
To traverse the model via a knowledge index
Update PutBucketLifecycleConfiguration test it now checks the Crc32 checksum header instead of Md5
Update HttpChecksumTest to have presigning types
Lockfile updates, comment formatting, bump version of crc64 crate
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
aws/sdk-codegen/src/test/kotlin/software/amazon/smithy/rustsdk/HttpChecksumTest.kt
Show resolved
Hide resolved
| // If we have made it this far without a checksum being set we set the default as Crc32 | ||
| let checksum_algorithm = incorporate_custom_default(state.checksum_algorithm, cfg) | ||
| .unwrap_or(ChecksumAlgorithm::Crc32); | ||
| // If we have made it this far without a checksum being set we set the default (currently Crc32) |
There was a problem hiding this comment.
Currently implies we could change it in the future, would that be behind a behavior version or do we think we can just change it without any concern in the future?
There was a problem hiding this comment.
Had the same question in my mind and my interpretation was to use a behavior version in that case. Programmatically, this PR adds [default] to ChecksumAlgorithm but we can switch the default to a different enum variant without breaking since aws-smithy-checksums is an unstable crate.
There was a problem hiding this comment.
The default is not specified by the SEP, so changing it is up to us. I don't think it necessarily would need a behavior version since all of this behavior is opaque to end users, the only difference they might notice is a slight performance change with the new default algorithm. Since any default change would be preceded by performance testing to ensure that the new algorithm is equally or more performant than the old one, I don't see it being an issue.
There was a problem hiding this comment.
Counterpoint from a HN comment: I guess sometimes the "more performant" algorithm can also be less performant for customers in more constrained environments. https://news.ycombinator.com/item?id=42691607
I think we are still ok not. putting this behind a behavior version, but worth thinking about.
…odes (#3964) <!--- Why is this change required? What problem does it solve? --> <!--- If it fixes an open issue, please link to the issue here --> awslabs/aws-sdk-rust#1234 <!--- Describe your changes in detail --> PR adds a new interceptor registered as part of the default retry plugin components that ensures a token bucket is _always_ present and available to the retry strategy. The buckets are partitioned off the retry partition (which defaults to the service name and is already set by the default plugin). We use a `static` variable in the runtime for this which means that token buckets can and will apply to every single client that uses the same retry partition. The implementation tries to avoid contention on this new global lock by only consulting it if the retry partition is overridden after client creation. For AWS SDK clients I've updated the default retry partition clients are created with to include the region when set. Now the default partition for a client will be `{service}-{region}` (e.g. `sts-us-west-2`) rather than just the service name (e.g. `sts`). This partitioning is a little more granular and closer to what we want/expect as failures in one region should not cause throttling to another (and vice versa for success in one should not increase available quota in another). I also updated the implementation to follow the SEP a little more literally/closely as far as structure which fixes some subtle bugs. State is updated in one place and we ensure that the token bucket is always consulted (before the token bucket could be skipped in the case of adaptive retries returning a delay and the adaptive rate limit was updated in multiple branches). <!--- Please describe in detail how you tested your changes --> <!--- Include details of your testing environment, and the tests you ran to --> <!--- see how your change affects other areas of the code, etc. --> <!--- If a checkbox below is not applicable, then please DELETE it rather than leaving it unchecked --> - [x ] For changes to the smithy-rs codegen or runtime crates, I have created a changelog entry Markdown file in the `.changelog` directory, specifying "client," "server," or both in the `applies_to` key. - [ x] For changes to the AWS SDK, generated SDK code, or SDK runtime crates, I have created a changelog entry Markdown file in the `.changelog` directory, specifying "aws-sdk-rust" in the `applies_to` key. ---- _By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice._
landonxjames
force-pushed
the
feature/flexible-checksums
branch
from
1f256f9 to
4b6aa8a
Compare
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
github-merge-queue
bot
removed this pull request from the merge queue due to a conflict with the base branch
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
Motivation and Context
Flexible checksums V2 will be launching this week
Description
Testing
Checklist
.changelogdirectory, specifying "client," "server," or both in theapplies_tokey..changelogdirectory, specifying "aws-sdk-rust" in theapplies_tokey.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.