Skip to content
This repository has been archived by the owner on Jul 27, 2022. It is now read-only.

feat: add support for range requests #15

Merged
merged 2 commits into from
May 24, 2022
Merged

feat: add support for range requests #15

merged 2 commits into from
May 24, 2022

Conversation

tustvold
Copy link
Contributor

@tustvold tustvold commented May 23, 2022

This adds support for range requests, unfortunately:

I personally wonder about just rolling our own GCS crate, it is a really straightforward API, with simple token-based auth, and #14 also runs into limitations of this crate

.err_into()
.boxed();
Ok(GetResult::Stream(
self.get_object(location, None).await?.boxed(),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved to get_object

format!("bytes={}-{}", range.start, range.end.saturating_sub(1))
}

/// Collect a stream into [`Bytes`] avoiding copying in the event of a single chunk
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a trick copied from hyper

Copy link
Contributor

@crepererum crepererum left a comment

Choose a reason for hiding this comment

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

Looks good (minor nits only), thanks for taking care of this.

Comment on lines +60 to +62
/// Return the bytes that are stored at the specified location
/// in the given byte range
async fn get_range(&self, location: &Path, range: Range<usize>) -> Result<Bytes>;
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the expected behavior on out-of-range requests? Do they error or soft-cap at the end of the object? Would probably be nice to add a test for that behavior.

src/lib.rs Outdated
Comment on lines 270 to 272
// Azurite doesn't support range requests
let err = range_result.unwrap_err().to_string();
assert!(err.contains("x-ms-range-get-content-crc64 header or parameter is not supported in Azurite strict mode"), "{}", err);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there an upstream ticket for that?

src/util.rs Outdated
Comment on lines 14 to 18
/// Returns the range to be passed to an object store
#[cfg(any(feature = "aws"))]
pub fn format_range(range: std::ops::Range<usize>) -> String {
format!("bytes={}-{}", range.start, range.end.saturating_sub(1))
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is AWS-specific, no? If it's specific, it should probably live within the AWS module. If it's not AWS-specific but follows some common HTTP standard, it would be nice to maybe rename the function to format_http_range and cite a standard.

@crepererum
Copy link
Contributor

I personally wonder about just rolling our own GCS crate

I think for the limited subset of functionality I would support that, not only for GCP. All the official upstream crates just bring a massive amount of dependencies.

@tustvold tustvold added the automerge Put in the merge queue label May 24, 2022
@kodiakhq kodiakhq bot merged commit 448434b into main May 24, 2022
@alamb alamb deleted the range-requests branch May 24, 2022 18:34
@tustvold tustvold mentioned this pull request May 26, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
automerge Put in the merge queue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants