Page MenuHomePhabricator

[blob-service] Support HTTP Range header
ClosedPublic

Authored by patryk on Jun 29 2023, 4:14 AM.
Tags
None
Referenced Files
F2135397: D8367.id28295.diff
Fri, Jun 28, 11:03 AM
F2135396: D8367.id28265.diff
Fri, Jun 28, 11:02 AM
F2135395: D8367.id28254.diff
Fri, Jun 28, 11:02 AM
F2135394: D8367.id28252.diff
Fri, Jun 28, 11:02 AM
F2135393: D8367.id28249.diff
Fri, Jun 28, 11:02 AM
F2135392: D8367.id.diff
Fri, Jun 28, 11:02 AM
F2135369: D8367.diff
Fri, Jun 28, 11:02 AM
Unknown Object (File)
Wed, Jun 26, 11:28 AM
Subscribers

Details

Summary

https://linear.app/comm/issue/ENG-3746/blob-service-support-http-range-header-for-get-blob-handler. Added
support for HTTP Range header. This header is valid with RFC-7233 standard.

Test Plan

I've run blob service and tested request via Postman. Tested ranges:
bytes=0-1023
bytes=5-
bytes=-10
bytes=0-99999 // Out of range

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

bartek requested changes to this revision.Jun 29 2023, 4:59 AM

The logic looks correct, but I have a few nits regarding code style ;)

services/blob/src/http/handlers/blob.rs
66 ↗(On Diff #28249)

Maybe we could extract this match to a separate function:

/// Returns a tuple of first and last byte number (inclusive) represented by given 
fn parse_range_header(range_header: Option<web::Header<Range>>, file_size: u64) -> actix_web::Result<(u64, u64)> {
 ....
}

What do you think?

78 ↗(On Diff #28249)

Unnecessary empty line (the same comment in a few places below)

105–106 ↗(On Diff #28249)

We can move this line down inside the if range_header.is_some() { ... } block

107 ↗(On Diff #28249)

This line can be right above the aforementioned if block where it is used

This revision now requires changes to proceed.Jun 29 2023, 4:59 AM

We could merge the two top-most matches and remove unnecessary clone like this:

let (range_start, range_end): (u64, u64) = match &range_header {
    Some(web::Header(Range::Bytes(ranges))) => {
      if ranges.len() > 1 {
        return Err(ErrorBadRequest("Multiple ranges not supported"));
      }
      <...rest of the code...>
   }
   Some(web::Header(Range::Unregistered(..))) => {
      return Err(ErrorBadRequest("Use ranges registered at IANA"));
    }
    None => (0, file_size - 1),
};

which I personally find a little easier to read

We could merge the two top-most matches and remove unnecessary clone like this:
...
which I personally find a little easier to read

Yes, good call. That is indeed an improvement worth doing

Moved code to separate function + styling changes.

Nit: I don't we need the two // Parse range header comments, now that we have a function with the same name

services/blob/src/http/handlers/blob.rs
127 ↗(On Diff #28252)

Unnecessary blank line

Merged the two top-most matches in parse_range_header function (as @michal suggested), removed unnecessary comments and
white blanks.

A small thing, and shouldn't matter performance-wise, but you should be able to take the range_header variable as a reference in the function, instead of taking it as an owned value and cloning it with to_owned().

In general, it's better to take references as arguments in functions (except for the Copy types) if it is possible. As a caller, you can always go from owned value to a reference, but not always the other way around or it could cost an additional clone.

services/blob/src/http/handlers/blob.rs
26–30 ↗(On Diff #28254)
107 ↗(On Diff #28254)

Nothing stands out to me that already hasn't been addressed.

Using reference instead of clone.

bartek added 1 blocking reviewer(s): michal.

Looks good to me,
Let @michal take a final look

This revision is now accepted and ready to land.Jun 30 2023, 2:18 AM