Conversation
Implements single-file Upload and Download for the SEP-compliant S3 Transfer Manager (aws-cpp-sdk-s3-transfer), backed by aws-c-s3 meta requests via the aws-crt-cpp S3 bindings. Covers file and stream sources/sinks, progress and finish callbacks, checksum config, cancellation, and CRT-to-SEP error mapping.
| // Sole real constructor; the public overloads adapt their credentials shape into the | ||
| // AWSCredentialsProvider taken here. A null endpointProvider means "use the default | ||
| // S3EndpointProvider", matching generated-client behavior. | ||
| S3TransferManagerImpl(const std::shared_ptr<Aws::Auth::AWSCredentialsProvider>& credentialsProvider, |
There was a problem hiding this comment.
"null endpointProvider means "use the default S3EndpointProvider"
Never rely on null for behavior, however always check for null. this sounds like a perfect case for default parameters
S3TransferManagerImpl(const S3TransferManagerConfiguration& config,
const std::shared_ptr<Aws::Auth::AWSCredentialsProvider>& credentialsProvider = Aws::MakeShared<DeafultCredentialsChain>(...),
const std::shared_ptr<Aws::S3::Endpoint::S3EndpointProviderBase>& endpointProvider = Aws::MakeShared<S3EndpointProvider>(...))since it is a internal header we dont need to worry about #include's as much because this will ultimately be compiled away for a user.
| private: | ||
| Aws::String m_region; | ||
| std::shared_ptr<Aws::Crt::Auth::ICredentialsProvider> m_credentialsProvider; | ||
| std::shared_ptr<Aws::S3::Endpoint::S3EndpointProviderBase> m_endpointProvider; |
There was a problem hiding this comment.
any reason these need to be shared and not unique?
| Aws::Utils::Threading::Executor& GetExecutor() const { return *m_executor; } | ||
|
|
||
| private: | ||
| Aws::String m_region; |
There was a problem hiding this comment.
if this is the region from S3TransferManagerConfiguration in the constructor? you should likely just make the member S3TransferManagerConfiguration instead of a string. Adding more to the configuration and making a decision on it is almost a guaruntee.
| } | ||
|
|
||
| private: | ||
| mutable std::mutex m_metaRequestLock; |
There was a problem hiding this comment.
is m_metaRequestLock referenced in a const method? if not why is it mutable?
| // synchronize through m_metaRequestLock so a cancel that races publication is never lost. | ||
| template <typename RequestT, typename OutcomeT> | ||
| struct AWS_CORE_LOCAL TransferStateBase { | ||
| std::promise<OutcomeT> promise; |
There was a problem hiding this comment.
public member variables means that anyone can edit them, should create setters/getter for the ones that need to be expose, and expose them properly, anything else should be scoped to the class they live in
| Aws::Crt::ByteCursor bytes, uint64_t rangeStart) noexcept; | ||
|
|
||
| S3DownloadBuffer(const S3DownloadBuffer&) = default; | ||
| S3DownloadBuffer& operator=(const S3DownloadBuffer&) = default; |
There was a problem hiding this comment.
why does this need to be copyable?
| * config. If config is not specified, it will be initialized to default values. | ||
| */ | ||
| S3TransferManager(const S3TransferManagerConfiguration& config = S3TransferManagerConfiguration(), | ||
| std::shared_ptr<Aws::S3::Endpoint::S3EndpointProviderBase> endpointProvider = nullptr); |
There was a problem hiding this comment.
same comment as above, do use null as default, use the the default endpoint provider
| */ | ||
| class AWS_S3_TRANSFER_API UploadResponse final { | ||
| public: | ||
| // Default constructor exists to satisfy Aws::Utils::Outcome<R, E>, which default-constructs |
There was a problem hiding this comment.
ugh i hate that, but so it goes
| }}, | ||
| Aws::Crt::ApiAllocator()); | ||
| if (!m_credentialsProvider) { | ||
| AWS_LOGSTREAM_ERROR(S3_TRANSFER_LOG_TAG, "Failed to create CRT credentials provider delegate."); |
There was a problem hiding this comment.
so lets think about something here, at this point th constructed object is literally unusable, period, what is the mode of error to the customer that it is unusable? it looks like we could just call it and it would try to make a call. theres a few ways around surfacing this error:
create a state object that will tell you if you class is borked or not
class widget {
public:
widget() {
state_ = STATE::ERROR;
}
std::expected<...> do_something() {
if (state_ == STATE::ERROR) {
return std::expected<...>
}
}
private:
enum class STATE {
INITIALZED,
ERROR,
} state_;
};make a factory method the returns a optional if it is actually useable
class widget {
public:
static std::expected<widget, std::error_code> create_widget() {
if (...) {
return std::unexpected<...>
}
widget widget{};
}
private:
widget() {}
};3 participants
Issue #, if available:
Description of changes:
Single File Upload/Download for TM 2.0
Check all that applies:
Check which platforms you have built SDK on to verify the correctness of this PR.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.