Page MenuHomePhabricator
Differential D5679

[services][blob] Introduce S3 primitives
ClosedPublic
Actions

Authored by bartek on Nov 18 2022, 9:41 AM.
Tags
None
Referenced Files
F3504807: D5679.diff
Fri, Dec 20, 10:32 AM
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)
Sat, Dec 7, 12:25 PM
Unknown Object (File)
Nov 7 2024, 5:45 AM
View All 97 Files
Subscribers
ashoat
atul
tomek

Details

Reviewers
• jon
varun
tomek
marcin
Commits
rCOMM7c865a63f7ec: [services][blob] Introduce S3 primitives
Summary

Introduced two S3 primitives:

  • S3 path - a convenience struct to store object and bucket name. See C++ counterpart
  • Multipart upload session - needed for Put requests to upload large blobs to S3. See C++ counterpart. Its implementation is added in the subsequent diff.

Depends on D5678

Test Plan

Added two simple unit tests for S3Path.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

bartek created this revision.Nov 18 2022, 9:41 AM
bartek held this revision as a draft.
Herald added a reviewer: • jon. · View Herald TranscriptNov 18 2022, 9:41 AM
Herald added subscribers: atul, tomek, ashoat. · View Herald Transcript
bartek added reviewers: varun, tomek, marcin.Nov 18 2022, 9:41 AM
Harbormaster completed remote builds in B13568: Diff 18587.Nov 18 2022, 9:54 AM
bartek added a child revision: D5680: [services][blob] Implement S3 multipart upload.Nov 18 2022, 9:57 AM
bartek published this revision for review.Nov 18 2022, 10:38 AM
ashoat added 1 blocking reviewer(s): varun.Nov 18 2022, 12:58 PM

Would be great if @varun could review all of @bartek's work on the blob service in Rust, given it touches AWS. @bartek, if @varun doesn't respond on diffs feel free to ping him on Comm

tomek requested changes to this revision.Nov 22 2022, 7:09 AM
tomek added inline comments.
services/blob/src/s3.rs
17 ↗(On Diff #18587)

It is almost never a good idea to panic in the code. We should handle errors more gracefully, by e.g. returning Result.

29 ↗(On Diff #18587)

It's usually more convenient to use format! macro

52 ↗(On Diff #18587)

You can probably use a shorthand: Result<(), anyhow::Error> should work the same as Result<()>

63 ↗(On Diff #18587)

It might be a good idea to add a test with invalid path

This revision now requires changes to proceed.Nov 22 2022, 7:09 AM
tomek added inline comments.Nov 22 2022, 9:24 AM
services/blob/src/s3.rs
13–14 ↗(On Diff #18587)

Why do we use triple slashes?

bartek added inline comments.Nov 23 2022, 3:45 AM
services/blob/src/s3.rs
13–14 ↗(On Diff #18587)

Rustdoc - IDE displays a pretty description when you hover on a function name.

Counterpart of

/**
 * Some description
 */

in JS

17 ↗(On Diff #18587)

I panicked, because I'm sick of this Option/Result unwrapping hell. But in this case, this shouldn't be a problem so I'll change that

52 ↗(On Diff #18587)

Yes, you're right. I'll change that everywhere

63 ↗(On Diff #18587)

Yes, I'll add it

bartek updated this revision to Diff 18801.Nov 24 2022, 12:01 AM
  • Used short form of anyhow::Result
  • The S3Path::from_full_path now returns Result
  • Used format! macro
  • Added a unit test for invalid path
Harbormaster completed remote builds in B13721: Diff 18801.Nov 24 2022, 12:11 AM
tomek accepted this revision.Nov 24 2022, 1:43 AM
tomek added inline comments.
services/blob/src/s3.rs
34 ↗(On Diff #18801)
13–14 ↗(On Diff #18587)

Ok, that makes sense!

varun accepted this revision.Nov 28 2022, 10:55 PM
This revision is now accepted and ready to land.Nov 28 2022, 10:55 PM
varun added inline comments.Nov 28 2022, 11:50 PM
services/blob/src/s3.rs
17 ↗(On Diff #18587)

I think Rust now supports let-else statements which might feel more ergonomic to you for unpacking Results and Options

bartek added inline comments.Nov 29 2022, 12:04 AM
services/blob/src/s3.rs
17 ↗(On Diff #18587)

Yes, since 1.65 (we're using 1.61 IIRC) - there are tasks for this: ENG-2002, ENG-2003

Closed by commit rCOMM7c865a63f7ec: [services][blob] Introduce S3 primitives (authored by bartek). · Explain WhyNov 29 2022, 5:15 AM
This revision was automatically updated to reflect the committed changes.
bartek added a commit: rCOMM7c865a63f7ec: [services][blob] Introduce S3 primitives.

Revision Contents
Changeset List

PathSize
services/
blob/
src/
1 line
95 lines

Diff 18935

View Options

services/blob/src/main.rs

Loading...
View Options

services/blob/src/s3.rs

Loading...