Skip to content

Conversation

@aldy505
Copy link
Contributor

@aldy505 aldy505 commented Nov 15, 2025

Closes #127

@aldy505 aldy505 requested a review from a team as a code owner November 15, 2025 10:29
mimalloc = { version = "0.1.48", features = ["v3", "override"] }
rand = "0.9.1"
reqwest = { version = "0.12.22" }
rust-s3 = "0.37.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

there is the "official" aws-sdk-s3, but its bloatware just like all other AWS packages.

///
/// - `OS__HIGH_VOLUME_STORAGE__REQUEST_TIMEOUT_SECS=30`
/// - `OS__LONG_TERM_STORAGE__REQUEST_TIMEOUT_SECS=30`
request_timeout_secs: Option<u64>,
Copy link
Contributor

Choose a reason for hiding this comment

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

this should probably be a Duration, with a humantime annotation.

Copy link
Contributor Author

@aldy505 aldy505 Nov 17, 2025

Choose a reason for hiding this comment

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

TIL humantime has an annotation.

Copy link
Contributor

Choose a reason for hiding this comment

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

#[allow(dead_code)]
pub extra_headers: HeaderMap,
pub request_timeout: Option<Duration>,
pub path_style: Option<bool>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Option<bool> is kind of an antipattern, so lets just make this a bool.

_path: &ObjectPath,
response: ResponseData,
) -> Result<Option<(Metadata, BackendStream)>> {
let metadata = Metadata::from_hashmap(response.headers(), "").unwrap_or_default();
Copy link
Contributor

Choose a reason for hiding this comment

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

this should propagate any error that happens.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wonder how we should propagate it, as I don't want it to return any error if there are any failure during parsing Metadata. I also have no idea on how error handling works on objectstore since we're using anyhow.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would say we want the correct metadata, or an error. Silently getting empty metadata would be wrong here.
And you can just propagate with ? like any other error.

Comment on lines +97 to +98
let bytes = Bytes::from(response.to_vec());
let stream: BackendStream = Box::pin(stream::iter(std::iter::once(Ok(bytes))));
Copy link
Contributor

Choose a reason for hiding this comment

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

S3 will most likely be used for large payloads, and we do not want to buffer those in memory. There should be some kind of way to get a stream from the ResponseData.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is actually: https://docs.rs/rust-s3/latest/s3/bucket/struct.Bucket.html#method.get_object_stream

But it's only compatible with async File IO type. I'm struggling to convert it into reqwest's streaming format.

Copy link
Contributor

Choose a reason for hiding this comment

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

tokio_util::io::ReaderStream is the struct that converts from an AsyncRead to a stream, like here:

https://github.com/getsentry/foundational-storage/blob/fac090250516932d09b9937ec4417c73475e07b8/objectstore-service/src/backend/local_fs.rs#L86

Though I know all these conversions can be a mess, so maybe this takes a bit of fiddling around :-(

/// Extracts metadata from the given [`HashMap`].
///
/// A prefix can be also be provided which is being stripped from custom non-standard headers.
pub fn from_hashmap(hash_map: HashMap<String, String>, prefix: &str) -> Result<Self, Error> {
Copy link
Member

Choose a reason for hiding this comment

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

This is almost identical to the method above, let's try to avoid the duplication

@jan-auer jan-auer marked this pull request as draft November 19, 2025 15:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support AWS v4 authentication for S3-compatible backend

3 participants