TM Header Update

[narahavi]

Issue #, if available:

Description of changes:

Check all that applies:

• Did a review by yourself.
• Added proper tests to cover this PR. (If tests are not applicable, explain.)
• Checked if this PR is a breaking (APIs have been changed) change.
• Checked if this PR will not introduce cross-platform inconsistent behavior.
• Checked if this PR would require a ReadMe/Wiki update.

Check which platforms you have built SDK on to verify the correctness of this PR.

• Linux
• Windows
• Android
• MacOS
• IOS
• Other Platforms

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[sbiscigl]

why std::enable_shared_from_this, what we are doing here is saying "the only way to create a transfer manager is via a factory that creates a shared pointer". we do that in the previous transfer manager, why are we doing that again?

if possible we should allow to create a stack variable instead of requiring shared pointer

[sbiscigl]

copy ctor delete foresure, but why delete move?

[sbiscigl]

so looking at this class you modeled it after most of our model classes that are codegenned. what i want to push back here on is do we need that? why are we following a builder patter when you could just have a constructor with default values? i.e.

class UploadRequest {
  public:
    explicit UploadRequest(...): m_s3Request{...}
  private:
     Aws::S3::Model::PutObjectRequest m_s3Request;
     Aws::String m_sourceFilePath;
     std::shared_ptr<Aws::IOStream> m_body;
     Aws::Vector<std::shared_ptr<UploadProgressListener>> m_transferListeners;
}

because what you are doing right now is allowing someone to default construct this without a request, source file path or body, and if those are not set the request should fail. if possible we should cover up ways to mis-use this with design patterns.

[sbiscigl]

why are we calling this SetBody its not really setting the body, its setting a output stream factory, the reason this factory exists is so that we have a 0 copy option when downloading data from remote. this sort of goes back to our conversation of "how do this interact with aws-c-s3 to achieve 0 copy", so what is your plan with using this stream factory?

[sbiscigl]

clang-tidy: https://clang.llvm.org/extra/clang-tidy/checks/readability/named-parameter.html all parameters must be named

[sbiscigl]

why shared_future? we typically use use std::future to enforce that the thread calling it has to resolve it

[sbiscigl]

why shared_ptr and not unqiue?

[sbiscigl]

theres a rule here either declare the destructor virtual, or make the class final so i cannot be inherited from. if someone were to subclass this and refer to it at the base, it would get sliced. so as a rule of the thumb, always add a virutal destructor or make the class final

[sbiscigl]

kinda weird to have a class that is only one member, since this is the first thing we do ill let it go for now but we should revist this existence if it only has one member

[sbiscigl]

use using over type def cpp core guidelines

[sbiscigl]

DownloadProgressSnapshot and UploadProgressSnapshot are about the same thing, we should likely break that out to a template i.e.

template<typename ReponseT>
class ProgressShapshot {
  ....
}

using DownloadProgressSnapshot = ProgressShapshot<ownloadResponse>;

etc