Migrate from rusoto to aws-sdk-rust

[josb]

https://github.com/awslabs/aws-sdk-rust is shaping up to be a great long-term replacement for https://github.com/rusoto/rusoto; this project should consider migrating to it.

[flub]

Agreed, it would be good to stay on a maintained library for the future and rusoto has indicated they will stop development once the aws-sdk-rust is stable. So while this isn't urgent it makes sense in the long term.

@josb would you be up for contributing a PR for this?

[josb]

@flub this seems like a good project to further hone my Rust skills. Challenge accepted. :-)

[josb]

FYI I'm still working on this, albeit slowly.

[flub]

Great to hear! I was just wondering about this task because of #728 which may or may not be about not speaking to S3 correctly.

[josb]

I just chatted with @mk4437 about this, will bump in priority. Stay tuned.

[josb]

A quick update: this is current stuck on 1 compilation error which seems to center around the HTTP response body being a different type between Rusoto and aws-sdk-rust. Haven't figured out a way to fix this yet.

[flub]

If you have a branch somewhere we can check out I wouldn't mind having a look at it.

[josb]

Sure! https://github.com/josb/symbolicator/tree/task/aws-sdk-rust-migration. Let me know what you think.

[flub]

I think you can use futures::TryStreamExt's .map_err() on the stream to change the error type of the stream. The larger problem seems to be that get_s3_client keeps a mutex locked across .await points, which you can't do. I don't think you need to keep it locked all that time though, you can probably make that shorter by locking it to get the item out and again to put one in.

[josb]

Thanks for the pointers, @flub. I'm currently struggling with this approach:

    async fn get_s3_client(&self, key: &Arc<S3SourceKey>) -> Arc<aws_sdk_s3::client::Client> {
        if let Some(client) = {
            let mut container = self.client_cache.lock();
            container.get(&*key)
        } {
            metric!(counter("source.s3.client.cached") += 1);
            client.clone()
        } else {
        ...

which yields an error[E0597]: container does not live long enough. Not sure how to get the value out while using the limited scope to automatically unlock the MutexGuard?

This isn't a problem with the put below:

            {
                let mut container = self.client_cache.lock();
                container.put(key.clone(), s3.clone())
            };

Any suggestions?

[flub]

huh, I take my earlier advice back. I looked a bit at the current and your version and instead I think you'll be better off changing the Mutex to be from tokio::sync instead of from parking_lot. This should at least get you further, I'm not fully happy with having the lock across wait points but for now I don't think we should re-design this as part of this PR.

(I'd be tempted to remove the lock and instead spawn a task responsible for the cache, send and retrieve clients from it over channels.)

[flub]

Any suggestions?

Another response is that you might have forgotten to clone the cache item before returning it. If you keep this way you avoid having a mutex over await points, which is good, but you'll end up with racing creation of creating of new clients, which is probably fine.

[josb]

Thanks, @flub ! What do you think of this solution?

[flub]

yep, that works fine too i think

[josb]

https://github.com/josb/symbolicator/tree/task/aws-sdk-rust-migration now compiles without warnings. Up next: merge master and try to fix the stuff I commented out (mostly tests).

Also:

• The aws-sdk-rust doesn't support custom regions; not sure what to do about yet, so the relevant test is commented out for now.
• download_source()in s3.rs has a bunch of custom error handling which I'll need to port to the new code; not sure how to go about that yet.

[josb]

FYI, I'll return to working on this as time permits, hopefully I'll have an update soon.