Integrate sync with s3transfer

[kyleknap]

So there is actually not much happening in this PR other than flipping sync to use the new architecture. Most of the code being added is actually from this PR: #2147.

So I was actually surprised that had to make this little of a change to get this to work because I do not at all take into account source clients when I do sync across buckets of different regions with a --delete. This is because we have auto-redirection now by default in botocore for sigv4. The question now becomes is the auto-redirection good enough to handle that use case? Also is there any other edge cases that I miss by not providing a source client for deletes. The only other edge case I could think of is kms, but for deletes of kms sigv4 is not needed.

The only reason I did not add support for providing support for a source client is that client's are attached to their TransferManager. There is currently no way to swap the two out. I thought about what I would do and these were the two best options I can think of:

1. Add future.meta.provide_client() to allow you to provide a different client for a particular transfer
2. Use two different transfer managers: one for the regular client and one for the source. Note I do not think that one is a good idea because we may be having separate executor thread pools which could break our configuration contract.

Option 1 or something along the lines of option 1 is the most realistic option. Option 2 is not really realistic at all.

Let me know what you think. Given that we cache the region at the client level and the integration test pass, I am fine with relying on redirection as opposed to having to add a new abstraction.

cc @jamesls @JordonPhillips

[jamesls]

Yeah, I'm surprised how little code was needed to get sync working. IIRC, the bucket region redirector in botocore is not thread safe, so I'd assume in the worst case you'd get N retried/redirected requests initially (N being number of threads). Does that match the behavior you're seeing?

As for not having a source client, the only downside I can think of is that you may not be able to do cross account syncs (i.e source/dest buckets in different AWS accounts). This isn't something you could do previously, but without source clients I don't think we'd be able to implement this feature. So my guess is that option 1 is something we may need to eventually add.

[kyleknap]

That's a good point. I think some perf testing on this is worth it. I do not even have to use the this branch of the CLI commands. I could just run rm --recursive with sigv4 and the wrong region set to see what the difference is when the correct region is used.

As to supporting a source client, we actually do support use it for copies: https://github.com/aws/aws-cli/blob/integrate-s3transfer/awscli/customizations/s3/s3handler.py#L710-L714. It is just for the cross-region sigv4 enabled sync --delete case that we run into the need to do the region redirector.

[jamesls]

Ah that's right. I think if there's not really any noticeable difference (even in the case of a small number of files) in perf, then this approach is simpler and more robust.

[kyleknap]

I re-ran the tests after rebasing. They pass now. Merging.