Page MenuHomePhabricator

[services][blob] Upload 1/2 - Introduce Put handlers
ClosedPublic

Authored by bartek on Nov 22 2022, 1:54 PM.
Tags
None
Referenced Files
F3496718: D5703.diff
Thu, Dec 19, 2:18 PM
Unknown Object (File)
Sun, Dec 15, 5:51 PM
Unknown Object (File)
Sun, Dec 15, 5:51 PM
Unknown Object (File)
Sun, Dec 15, 5:51 PM
Unknown Object (File)
Sun, Dec 15, 5:51 PM
Unknown Object (File)
Sun, Dec 15, 5:51 PM
Unknown Object (File)
Sun, Dec 15, 5:51 PM
Unknown Object (File)
Sun, Dec 15, 5:51 PM
Subscribers

Details

Summary

Support for uploading blobs with Put RPC call. This is the most complicated part of the service as it is stateful.

This is part 1 of 2

I implemented it slightly differently from the C++ counterpart. I created a separate PutHandler struct to organize the code. A few notes on how it works:

  • In each input stream message, client can send either holder, blob hash or data chunk.
  • Put handler starts with a PutAction::None state and waits for both holder and hash to be provided (in any order, but each only once).
  • When both of them exist, next state is determined:
    • If blob hash already exists, the AssignHolder action is scheduled and the input stream is closed by the server.
    • Otherwise, the UploadNewBlob action is initialized and handler is waiting for data chunks

Here is where this diff ends, further actions are in the next diff

  • (next diff) Messages containng data chunks can only be sent when handler is in the UploadNewBlob state. Chunks are uploaded to S3 if big enough. The Multipart upload session is initialized when first chunk is sent.
  • (next diff) After input stream is closed, the finish() method is invoked, which consumes the handler and performs action depending on the enum value:
    • If no action, just return
    • For AssignHolder - just add a new row to the reverse_index table
    • In case of UploadNewBlob - finishes the S3 upload and adds rows to both DB tables

Depends on D5702

Test Plan

Now all gRPC handlers are implemented so running integration tests against this service is possible.
In constants set port to 50053 and add the -test suffix to both table names, run the service, then run yarn run-integration-tests blob.

Diff Detail

Repository
rCOMM Comm
Branch
barthap/blob-rust
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

bartek held this revision as a draft.

This diff is still a draft. Is that intentional?

bartek published this revision for review.Nov 24 2022, 5:28 AM

This diff is probably too large for your standards, but I have no idea how to split it - the code here forms a tight whole.

Please limit all lines to 80 chars long

tomek requested changes to this revision.Nov 25 2022, 6:48 AM

This looks great and is a lot more readable than the original solution! Regarding splitting the diff, one option might be to handle chunks upload as a separate one - this makes sense as the first diff would be about setting up and determining the action, and the second one about executing it.

services/blob/src/service.rs
253 ↗(On Diff #18815)

Just wondering what would be more maintainable: PutAction::None or Option<PutAction> where None would represent what PutAction::None currently means.

265 ↗(On Diff #18815)

We can consider simplifying the API by storing db and s3 as instance variables

266 ↗(On Diff #18815)

Shouldn't we use &str here? Overall, using &str as parameter is the most convenient as it allows accepting a lot of different types of arguments.

298 ↗(On Diff #18815)

What is the purpose of data_exists flag?

358–361 ↗(On Diff #18815)

Is it possible that after appending we exceed the max upload chunk size? Will the uploader handle it correctly, or do we need to split it here?

This revision now requires changes to proceed.Nov 25 2022, 6:48 AM

Thanks for the suggestion regarding splitting the diff - sounds reasonable to me.

services/blob/src/service.rs
253 ↗(On Diff #18815)

I did a quick refactor and it doesn't make a big difference. It's only a matter of wrapping/unwrapping values with Some() here and there. One line is a bit more readable, another is a bit less. 🤷‍♂️

265 ↗(On Diff #18815)

Yes, this is worth doing. Thanks to Arc this shouldn't be a problem.

266 ↗(On Diff #18815)

I considered that, but my logic is that this function consumes the holder (moves it) so it shouldn't be used after it's called.
Also, maybe it doesn't matter here because it's just a tiny string, but in general, I try to avoid copying when unnecessary.

298 ↗(On Diff #18815)

A relict of current proto API. A false should be returned in all cases, except when a blob with provided blob_hash already exists - then it's assigned to the given holder and true is returned.

358–361 ↗(On Diff #18815)

Hmm...
I doubt it, because for GRPC max message size is 4MiB - 5 bytes (see constants in D5700) and for S3 the min size is 5MiB.

This implies that largest chunks uploaded from blob service will have 8MiB - 10 bytes (~8.4MB) so this is way too small for the uploader to cause any problems. According to the S3 docs, max single part size is 5GB.

bartek retitled this revision from [services][blob] Handle uploading new blobs to [services][blob] Upload 1/2 - Introduce Put handlers.Nov 28 2022, 1:36 AM
bartek edited the summary of this revision. (Show Details)
bartek edited the summary of this revision. (Show Details)
  • Split the diff
  • Stored s3 and db as PutHandler members
services/blob/src/service.rs
305–312 ↗(On Diff #18850)

Should I extract this to BlobItem::new(blob_hash)? (of course in another diff)

tomek added inline comments.
services/blob/src/service.rs
305–312 ↗(On Diff #18850)

Up to you - it would be an improvement, but keeping it as is, isn't terrible. So if you have some spare time, you can do it, but I wouldn't prioritize it.

253 ↗(On Diff #18815)

Ok, thanks for checking that! Let's use the more readable solution then.

  • Rebase
  • Used Option<PutAction> instead of having None as PutAction case, as found it a bit more readable in the child diff
ashoat requested changes to this revision.Nov 29 2022, 6:54 AM

Repeating this feedback:

Please limit all lines to 80 chars long

Please keep this mind for every line of every revision of every diff you put up. Ideally linters can help you (maybe ENG-2314 will help, cc @varun). But absent that, the responsibility is on you

This revision now requires changes to proceed.Nov 29 2022, 6:54 AM

Ideally linters can help you

cargo fmt didn't complain about this

varun requested changes to this revision.Nov 29 2022, 7:50 PM

good stuff! just one question inline

services/blob/src/service.rs
297 ↗(On Diff #18956)

can we avoid using unwrap here? i know we're already returning early if blob_hash is None but that logic could change and then we risk panicking here

This revision now requires changes to proceed.Nov 29 2022, 7:50 PM
services/blob/src/service.rs
272 ↗(On Diff #18956)

don't need the return keyword here

280 ↗(On Diff #18956)

don't need the return keyword here

298 ↗(On Diff #18956)

don't need the return keyword here

  • Removed unnecessary returns
  • Replaced unwrap() with returning error
varun requested changes to this revision.Nov 29 2022, 10:23 PM
varun added inline comments.
services/blob/src/service.rs
272 ↗(On Diff #18988)

pretty sure this won't compile since you need to .await determine_action(). same with line 280

This revision now requires changes to proceed.Nov 29 2022, 10:23 PM
services/blob/src/service.rs
272 ↗(On Diff #18988)

good catch, I added missing awaits after it failed and didn't save the file 🙈

This revision is now accepted and ready to land.Nov 30 2022, 6:23 AM