Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add range header in put_block_url function #1446

Merged
merged 4 commits into from
Oct 25, 2023
Merged

Conversation

shaopu1225
Copy link
Contributor

As described in document, the put_block_url function has a x-ms-source-range header request option.

@shaopu1225 shaopu1225 closed this Oct 25, 2023
@shaopu1225 shaopu1225 reopened this Oct 25, 2023
@shaopu1225
Copy link
Contributor Author

@microsoft-github-policy-service agree

@@ -23,6 +24,11 @@ impl PutBlockUrlBuilder {
let mut headers = Headers::new();
headers.insert(COPY_SOURCE, self.url.to_string());
headers.add(self.lease_id);
if let Some(mut range) = self.range {
// to accomodate with the standard HTTP range header format:
Copy link
Contributor

Choose a reason for hiding this comment

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

The current range implementation used by the SDK is intended to be an inclusive end.

For reference:

let cp_start = v[0].parse::<u64>().map_kind(ErrorKind::DataConversion)?;
let cp_end = v[1].parse::<u64>().map_kind(ErrorKind::DataConversion)? + 1;

We need to document this behavior and be more intentionally specific about it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure whether the current comitted version is correct...It's kind of weird for the user to give in a range of [0,1000] to obtain the range header bytes=0-999.

Copy link
Contributor

Choose a reason for hiding this comment

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

Understood. The current handling of Range is suboptimal.

Our range implementation maps more closely to RangeInclusive in practice. As identified in #1438, we need to clean this up in a handful of different places.

Copy link
Contributor

@demoray demoray left a comment

Choose a reason for hiding this comment

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

See comments

@demoray demoray self-requested a review October 25, 2023 17:16
@demoray demoray merged commit 4849d79 into Azure:main Oct 25, 2023
19 checks passed
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.

None yet

2 participants