Skip to content
This repository has been archived by the owner on Feb 28, 2023. It is now read-only.

s3 rusoto and general futures 0.3 migration of blobstore #3

Closed
wants to merge 60 commits into from

Conversation

drahnr
Copy link
Contributor

@drahnr drahnr commented Nov 12, 2020

TriplEight
TriplEight previously approved these changes Nov 12, 2020
Copy link
Contributor

@TriplEight TriplEight left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be merged as it's green in mozilla/sccache#869

@drahnr
Copy link
Contributor Author

drahnr commented Nov 12, 2020

Are we using s3 storage for our setup?

@TriplEight
Copy link
Contributor

Nope, that would be quite slow and expensive.
Though, there may be an option to organize our own s3 storage near the runners.

@drahnr
Copy link
Contributor Author

drahnr commented Nov 12, 2020

Hence I am asking, minio scales pretty well.

@drahnr drahnr changed the title s3 rusoto s3 rusoto and general futures 0.3 migration of blobstore Dec 3, 2020
src/bin/sccache-dist/token_check.rs Outdated Show resolved Hide resolved
];
let profile_provider =
ProfileProvider::with_configuration(home.join(".aws").join("credentials"), "default")
// //TODO: this is hacky, this is where our mac builders store their
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

double checking : are those double '//' '//' introduced intentionally?

Copy link
Contributor Author

@drahnr drahnr Dec 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, not at all.

runtime.block_on(server.with_graceful_shutdown(async move {
let _ = shutdown_signal;
}))
// .map_err(|e| {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you want to leave this code commented?

is it ok to not handle this in new code?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its a bit of a TODO..

@Xanewok
Copy link
Contributor

Xanewok commented May 11, 2021

We migrated to futures 0.3 in #68 however the simples3 implementation was translated directly, to reduce the review surface for the original PR at mozilla/sccache#985.

Are we still interested in Rusoto? It's worth noting that the project entered maintenance mode and it seems that the new official AWS SDK for Rust was recently announced and is in alpha stage now: https://aws.amazon.com/blogs/developer/a-new-aws-sdk-for-rust-alpha-launch/ (repo: https://github.com/awslabs/aws-sdk-rust)

@drahnr
Copy link
Contributor Author

drahnr commented May 17, 2021

Reducing the API surface was a very good call (thanks!).

Not sure how to proceed. rusoto has (I think) a few nice to have features.

I am bit wary of migrating to anything super-alpha mostly due to the potential breakage up the road, rusoto is likely going to be stable and stick around for quite some time.

It's your call essentially, not sure if other PRs like clean bucket depend on it.

Slight tendency to for rusoto, to simplify away some internal http chore we don't really care about.

@alnr
Copy link
Contributor

alnr commented Aug 2, 2021

Bump. Can this be merged?

@drahnr
Copy link
Contributor Author

drahnr commented Aug 2, 2021

Bump. Can this be merged?

This PR needs to be condensed down to the rusoto only changset, the features migration already landed in master. This is not a super high priority right now, but is also not that much work either and will be addressed once the first release makes it to crates.io .

@Xanewok
Copy link
Contributor

Xanewok commented Aug 10, 2021

@drahnr can you take a look at https://github.com/paritytech/cachepot/tree/igor-s3-rusoto? I adapted the changes from https://github.com/diem/sccache/tree/rusoto which AIUI the originally mentioned mozilla/sccache#869 uses. I didn't force update the branch in case there's something else we might need to port as well.

@Xanewok Xanewok mentioned this pull request Aug 10, 2021
@Xanewok Xanewok closed this in #99 Aug 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants