Skip to content

Conversation

@Velfi
Copy link
Contributor

@Velfi Velfi commented Apr 19, 2022

Motivation and Context

Implements the Body Callback RFC

Description

add: callback setting API to ByteStream
add: callback setting API to SdkBody
add: callback tests for streaming and buffered data
add: private callback fns for calculating checksums
udpate: pub methods on private struct Inner to be private
update: changelog
remove: Sync bound from SdkBody test "sdk_body_is_send()"
add: fn to merge HeaderMaps by appending
add: tests for new functionality

Testing

ran existing tests and created new ones

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.

add: callback setting API to ByteStream
add: callback setting API to SdkBody
add: callback tests for streaming and buffered data
add: private callback fns for calculating checksums
udpate: pub methods on private struct `Inner` to be private
update: changelog
remove: Sync bound from SdkBody test "sdk_body_is_send()"
add: fn to merge `HeaderMap`s by appending
add: tests for new functionality
@Velfi Velfi requested a review from a team as a code owner April 19, 2022 19:45
@Velfi
Copy link
Contributor Author

Velfi commented Apr 19, 2022

I'm putting this up for initial review. It has two TODOs that I could use some advising on. Currently, the checksum callbacks are private because I don't want people to start messing with them. This PR, once merged, will be followed by another PR that implements the codegen side of things and makes the callbacks public.

Zelda Hessler added 3 commits April 20, 2022 10:18
refactor: poll_trailers method
update: split out checksum callbacks into their own module
formatting: use throwaway let binding instead of allow unused variable
add: readme file to new crate
Copy link
Contributor

@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 great! I just have some minor comments and bike shedding.

Comment on lines +96 to +97
assert!(logs_contain("callback A was called"));
assert!(logs_contain("callback B was called"));
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we care what order these were called in? I don't think the log testing will inform on that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think no. I'm uncomfortable with users depending on callbacks running in a specific order because it just seems like that would be a source of bugs/confusion. This test exists to show that all callbacks get called when setting more than one.

Zelda Hessler added 3 commits April 20, 2022 14:12
add: two more append_merge_header_maps tests
update: make append_merge_header_maps pub(crate)
remove: check for non-existent log line
@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

Rust Wrk benchmark report:

Duration: 90 sec, Connections: 32, Threads: 2

Measurement Deviation Current Old
Requests/sec 0.46% 77680.92 77326.87
Total requests 0.43% 6995295 6965271
Total errors NaN% 0 0
Total successes 0.43% 6995295 6965271
Average latency ms 3.67% 1.13 1.09
Minimum latency ms 0.00% 0.02 0.02
Maximum latency ms 14.08% 24.47 21.45
Stdev latency ms 1.60% 1.91 1.88
Transfer Mb 0.43% 727.16 724.04
Connect errors NaN% 0 0
Read errors NaN% 0 0
Write errors NaN% 0 0
Status errors (not 2xx/3xx) NaN% 0 0
Timeout errors NaN% 0 0

@smithy-lang smithy-lang deleted a comment from github-actions bot Apr 21, 2022
@smithy-lang smithy-lang deleted a comment from github-actions bot Apr 21, 2022
@smithy-lang smithy-lang deleted a comment from github-actions bot Apr 21, 2022
@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

Rust Wrk benchmark report:

Duration: 90 sec, Connections: 32, Threads: 2

Measurement Deviation Current Old
Requests/sec 6.00% 38172.97 36010.66
Total requests 6.02% 3437743 3242585
Total errors NaN% 0 0
Total successes 6.02% 3437743 3242585
Average latency ms -3.26% 0.89 0.92
Minimum latency ms -33.33% 0.02 0.03
Maximum latency ms -1.03% 18.18 18.37
Stdev latency ms 10.00% 0.66 0.6
Transfer Mb 6.02% 357.36 337.07
Connect errors NaN% 0 0
Read errors NaN% 0 0
Write errors NaN% 0 0
Status errors (not 2xx/3xx) NaN% 0 0
Timeout errors NaN% 0 0

Copy link
Contributor

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

LGTM!

@Velfi Velfi merged commit 1372754 into main Apr 21, 2022
@Velfi Velfi deleted the feature/flexible-checksums-foundation branch April 21, 2022 17:51
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.

3 participants