Page MenuHomePhabricator
Differential D5701

[services][blob] Handle getting blob data
ClosedPublic
Actions

Authored by bartek on Nov 22 2022, 12:58 AM.
Tags
None
Referenced Files
F3529528: D5701.diff
Tue, Dec 24, 7:29 PM
Unknown Object (File)
Mon, Dec 16, 11:01 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:50 PM
Unknown Object (File)
Sun, Dec 15, 5:50 PM
Unknown Object (File)
Sun, Dec 15, 5:49 PM
View All Files
Subscribers
ashoat
atul

Details

Reviewers
varun
• jon
tomek
marcin
Commits
rCOMM26b79bb26eab: [services][blob] Handle getting blob data
Summary

Implemented basic logic for the Get RPC call.

C++ counterpart

Depends on D5700

Test Plan
  1. Create a blob using e.g. the C++ blob service (or in any other way)
  2. Get the blob data with the Rust implementation
  3. If the data is small, modify chunk_size to some small value to test if multiple parts are returned as expected

Diff Detail

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

Event Timeline

bartek created this revision.Nov 22 2022, 12:58 AM
bartek held this revision as a draft.
Herald added subscribers: atul, ashoat. · View Herald TranscriptNov 22 2022, 12:58 AM
bartek added a child revision: D5702: [services][blob] Handle removing blobs.Nov 22 2022, 1:03 AM
Harbormaster completed remote builds in B13655: Diff 18711.Nov 22 2022, 1:11 AM
bartek updated this revision to Diff 18725.Nov 22 2022, 5:42 AM

Rebase

Harbormaster completed remote builds in B13669: Diff 18725.Nov 22 2022, 5:55 AM
bartek published this revision for review.Nov 22 2022, 10:18 AM
varun requested changes to this revision.Nov 23 2022, 12:36 AM
varun added inline comments.
services/blob/src/service.rs
103 ↗(On Diff #18725)

what's happening here?

135 ↗(On Diff #18725)

Same feedback as previous diff about keeping the Status message brief and generic

This revision now requires changes to proceed.Nov 23 2022, 12:36 AM
bartek updated this revision to Diff 18813.Nov 24 2022, 5:24 AM

Made response errors vague, tried to make code a bit more readable

Harbormaster completed remote builds in B13730: Diff 18813.Nov 24 2022, 5:37 AM
tomek accepted this revision.Nov 25 2022, 5:01 AM
varun accepted this revision.Nov 28 2022, 11:31 PM

One question inline

services/blob/src/service.rs
90 ↗(On Diff #18813)

Do you think we should do some validation on the value returned by content_length before we take the absolute value?

This revision is now accepted and ready to land.Nov 28 2022, 11:31 PM
bartek added inline comments.Nov 29 2022, 12:38 AM
services/blob/src/service.rs
90 ↗(On Diff #18813)

Good catch, I can do sth like this:

let file_size: u64 = object_metadata.content_length().try_into().map_err(|err| {
  error!("Invalid content length: {:?}", err);
  Status::aborted("server error")
})?;
bartek updated this revision to Diff 18916.Nov 29 2022, 3:00 AM
  • Rebase
  • Validate received content length
Harbormaster completed remote builds in B13823: Diff 18916.Nov 29 2022, 3:13 AM
Closed by commit rCOMM26b79bb26eab: [services][blob] Handle getting blob data (authored by bartek). · Explain WhyNov 29 2022, 5:15 AM
This revision was automatically updated to reflect the committed changes.
bartek added a commit: rCOMM26b79bb26eab: [services][blob] Handle getting blob data.

Revision Contents
Changeset List

PathSize
services/
blob/
src/
78 lines

Diff 18711

View Options

services/blob/src/service.rs

Loading...