[v2] Add support for stdin/stdout streams for CRT client #8291
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## v2-s3-crt #8291 +/- ##
==========================================
Coverage 93.14% 93.15%
==========================================
Files 364 364
Lines 38482 38497 +15
Branches 6167 6167
==========================================
+ Hits 35845 35862 +17
+ Misses 1964 1962 -2
Partials 673 673
☔ View full report in Codecov by Sentry. |
awscli/s3transfer/crt.py
Outdated
| return getattr( | ||
| self, | ||
| f'_get_make_request_args_{request_type}', | ||
| self._default_get_make_request_args, | ||
| )( | ||
| request_type=request_type, | ||
| call_args=call_args, | ||
| coordinator=coordinator, | ||
| future=future, | ||
| on_done_before_calls=[], | ||
| on_done_after_calls=on_done_after_calls, |
There was a problem hiding this comment.
nit; this seems auto-magic enough to be difficult to debug if we have a failure in getattr or the function call itself since they'll all be on the same line. It may be worth splitting this into a variable for the function and then its invocation.
There was a problem hiding this comment.
That's reasonable. I'll break it up.
awscli/s3transfer/crt.py
Outdated
| # Suppress botocore's automatic MD5 calculation by setting an override | ||
| # value that will get deleted in the BotocoreCRTRequestSerializer. | ||
| # The CRT S3 client will automatically compute checksums as part of | ||
| # requests it makes. |
There was a problem hiding this comment.
CRT S3 Client doesn't compute checksums unless you pass an S3ChecksumConfig. See me setting the checksum algorithm and location here
So this (and the currently released versions) aren't doing any checksums or MD5 for uploads
There was a problem hiding this comment.
Gotcha. Yeah one of the follow ups is to automatically set checksums for these requests as you had it in your original PR. For now, I'll just delete/update the second sentence in that comment.
This also included: * Refactoring the s3transfer crt layer to better organize the branching introduced in supporting both file objects and file name. * Introduce additional helper test methods to reduce verbosity of s3transfer crt test and improve reusability.
kyleknap
force-pushed
the
v2-crt-nonseekable
branch
from
957fee0
to
e50b101
Compare
This is a backport of this pull request: aws/aws-cli#8291
[v2] Add support for stdin/stdout streams for CRT client
[v2] Add support for stdin/stdout streams for CRT client
[v2] Add support for stdin/stdout streams for CRT client
[v2] Add support for stdin/stdout streams for CRT client
This PR pulls in changes from this s3transfer PR: boto/s3transfer#277. However, it only pulls in a subset of changes proposed specific to streaming support. The other proposed changes from the s3transfer PR will be considered/pulled in via subsequent PRs. This PR currently targets a feature branch that will consist of all CRT transfer client improvements that we are looking to make, and this branch will eventually be merged into the
v2branch. Once merged, the improvements to thes3transfercode will be backported to the upstreams3transferproject.In supporting steams at the
s3transferCRT layer, I also did the following refactorings: