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
F3531513: D5701.id18916.diff
Wed, Dec 25, 7:27 AM
F3531498: D5701.id18944.diff
Wed, Dec 25, 7:22 AM
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
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
Lint
Lint Not Applicable
Unit
Tests Not Applicable

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/
77 lines

Diff 18944

View Options

services/blob/src/service.rs

Loading...