Page MenuHomePhabricator

[services][blob] Introduce S3 Client abstraction
ClosedPublic

Authored by bartek on Dec 1 2022, 5:01 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Dec 15, 5:50 PM
Unknown Object (File)
Sun, Dec 15, 5:50 PM
Unknown Object (File)
Sun, Dec 15, 5:50 PM
Unknown Object (File)
Sun, Dec 15, 5:49 PM
Unknown Object (File)
Sun, Dec 15, 5:29 PM
Unknown Object (File)
Nov 7 2024, 2:53 PM
Unknown Object (File)
Nov 7 2024, 5:48 AM
Unknown Object (File)
Nov 4 2024, 3:55 PM
Subscribers

Details

Summary

Follow up https://linear.app/comm/issue/ENG-2341/blob-service-create-s3-client-abstraction which follows up https://phab.comm.dev/D5682?id=18592#inline-38028

This diff introduces new struct S3Client which is a analogous to DynamoDB DatabaseClient. It's not yet used anywhere.

Test Plan

Project should still compile.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

bartek held this revision as a draft.
bartek published this revision for review.Dec 1 2022, 6:04 AM
tomek added inline comments.
services/blob/src/s3.rs
78 ↗(On Diff #19066)

I'm wondering if we can use some specific type, e.g. Range. It would look great to see e.g. get_object_bytes(path, (start..end)). Additional benefit would be to clarify the confusion about whether end value is inclusive. On the other hand, creating Range instance might be more expensive - we can check that.

services/blob/src/s3.rs
78 ↗(On Diff #19066)

Good point. Initially I thought about either just a typealias:

type Range = (u64, u64)

or maybe better a tuple struct:

struct Range(u64, u64)

But builtin Range should do the job. It shouldn't be any more expensive from a tuple, memory footprint should be the same as this is a two-integer struct.

The only problem with the built-in one is that in docs, end is marked as exclusive, but in fact S3 expects HTTP Range header format, which has inclusive end. Eventually, we could just subtract 1 from the value of end.

services/blob/src/s3.rs
78 ↗(On Diff #19066)

It looks like the start..=end expression produces a std::ops::RangeInclusive instance, which is another built-in struct for ranges.
So this is another solution to go, maybe the best as we can use the ..= expression which tells us that the range is inclusive.
Or we could be even more general and just require a std::ops::RangeBounds trait which accepts all ranges, including unbound ones.

services/blob/src/s3.rs
78 ↗(On Diff #19066)

Using RangeBounds sounds like a great idea! Just one concern is if we can support unbounded ranges. Can we specify an open ended HTTP Range?

This revision is now accepted and ready to land.Dec 6 2022, 8:01 PM

Changed range to be of type RangeBounds to support all possible ranges.