Add more `Store` implementations about alexandrie HOT 5 OPEN

I would tend to be in favor of making it be behind a feature flag.
Since the user already has to choose a database implementation using a feature flag, I would say that it is a price that we already payed.
Also, I think that handling features in workspaces is going to improve in usability in the future, according to this issue.

from alexandrie.

i think using artifactory for binary storage would be useful for a lot of enterprise users.

from alexandrie.

Hi, and thanks for this project! I'm evaluating it for internal use at my company, and I'd like to add support for S3 storage. I've started by trying to use rusoto_s3. Implementing the "get" methods is no problem, but implementing the "store" methods seems to be pretty painful:

• The put_object method wants a StreamingBody as its payload.
• Constructing a StreamingBody (which is a type alias for rusoto_core::ByteStream) appears to need a Stream<Item = Result<bytes::Bytes, std::io::Error>> + Send + Sync + 'static, which already seems difficult to satisfy with the current store type bound of data: T where T: io::Read, both because of the inherent pain of converting blocking -> streaming and the missing extra Send + Sync + 'static type bounds. But on top of that...
• Apparently S3 puts actually require you to know the size up front. So not only do we have all the type bound restrictions + sync -> async, we also have to know the full size ahead of time.

It seems like the only way to satisfy that is to read the full data into a Vec<u8>, which can trivially be converted into a ByteStream. I found another S3 library that has a method that takes a blocking std::io::Reader for puts, and that's exactly what it does.

Looking higher up the call stack, the store methods are called from alexandrie/src/crates/publish.rs, and it looks like they have already read the full data into memory. So passing a T: Read down to the store methods which would read them into a new vec would just be making a copy. That doesn't seem terrible, but doesn't seem ideal either. It looks like with a small amount of work, the caller could pass an owned Vec<u8> down to the store methods, which would avoid the double copy.

Do you have any interest in changing the store methods to take a Vec<u8> instead of a T: Read? Or would you rather keep T: Read but need to make a copy when storing to S3? (Or if you see another avenue out of this, that'd be great too!)

from alexandrie.

@jgallagher-cs Hello, and thank you for the interest and willingness to help !
It indeed would be really cool to have the ability to leverage S3 as a storage option.

Reading your comment made me realize that the Storage abstraction is trying to do more than what is actually needed from it.
You're completely right that we have no use for streaming bytes into the storage without having buffered the entire crate already.
I (most-likely) made the the mistake of adding this trait method as a way-too-early guess that it could be useful one day, but, as you've shown, even if it was something that is needed/desired, the current implementation is not the right one.

I am in favor of changing the API as you suggested, by simply taking a Vec<u8>.
I think it models better what the registry really does and needs.

from alexandrie.

👍 Great, I'll make that change.

Do you want S3 support gated behind a Cargo feature? It ends up pulling in quite a few dependencies, so I would assume "yes", but features don't really play nicely with workspaces (e.g., rust-lang/cargo#7507). The docs for building with S3 support would be something like

cd alexandrie
cargo build --release --features s3

:(

from alexandrie.