Page MenuHomePhabricator
Differential D5700

[services][blob] Add helpers and constants for Get RPC
ClosedPublic
Actions

Authored by bartek on Nov 22 2022, 12:53 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Dec 25, 8:11 AM
Unknown Object (File)
Fri, Dec 20, 3:12 PM
Unknown Object (File)
Fri, Dec 20, 2:31 PM
Unknown Object (File)
Fri, Dec 20, 2:29 PM
Unknown Object (File)
Fri, Dec 20, 1:59 PM
Unknown Object (File)
Fri, Dec 20, 11:10 AM
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
rCOMM936f80c0ba8c: [services][blob] Add helpers and constants for Get RPC
Summary
  • Added helper convenience functions to get S3 path from database
  • Copied constants from C++ service with comments

Depends on D5694

Test Plan

These functions are tested in next diff when they are used.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

bartek created this revision.Nov 22 2022, 12:53 AM
bartek held this revision as a draft.
Herald added subscribers: atul, ashoat. · View Herald TranscriptNov 22 2022, 12:53 AM
bartek added a child revision: D5701: [services][blob] Handle getting blob data.Nov 22 2022, 12:58 AM
Harbormaster completed remote builds in B13654: Diff 18710.Nov 22 2022, 1:06 AM
bartek updated this revision to Diff 18724.Nov 22 2022, 5:41 AM

Made S3 helpers return Result<S3Path, Status>

Harbormaster completed remote builds in B13668: Diff 18724.Nov 22 2022, 5:55 AM
bartek published this revision for review.Nov 22 2022, 8:29 AM
bartek edited the summary of this revision. (Show Details)
varun requested changes to this revision.Nov 23 2022, 12:27 AM

Think this could also be two or three diffs. No need to split it now, though.

You might have noticed that in the Identity service, the Status messages are vague (e.g. "internal error"). This is intentional, because we don't want to leak implementation details to the caller. I think we should amend all the Status messages in this diff to be much less specific. The current Status messages that you have should be logged instead of returned to the caller

services/blob/src/service.rs
60 ↗(On Diff #18724)

Do we really want to send the S3 path back to the caller? I doubt it

This revision now requires changes to proceed.Nov 23 2022, 12:27 AM
bartek updated this revision to Diff 18811.Nov 24 2022, 1:56 AM

Made error messages more vague. Internal logging will be added later (ENG-2308).

Harbormaster completed remote builds in B13728: Diff 18811.Nov 24 2022, 2:11 AM
tomek accepted this revision.Nov 24 2022, 5:01 AM
ashoat added a comment.Nov 24 2022, 9:35 AM

(For what it's worth, I think this is fine as one diff... splitting diffs too much can make it harder to review)

bartek mentioned this in D5728: [services][blob] Upload 2/2 - Handle data chunks.Nov 28 2022, 9:27 AM
varun accepted this revision.Nov 28 2022, 11:06 PM
This revision is now accepted and ready to land.Nov 28 2022, 11:06 PM
bartek updated this revision to Diff 18911.Nov 29 2022, 2:33 AM

Rebase

Harbormaster completed remote builds in B13818: Diff 18911.Nov 29 2022, 2:45 AM
Closed by commit rCOMM936f80c0ba8c: [services][blob] Add helpers and constants for Get RPC (authored by bartek). · Explain WhyNov 29 2022, 5:15 AM
This revision was automatically updated to reflect the committed changes.
bartek added a commit: rCOMM936f80c0ba8c: [services][blob] Add helpers and constants for Get RPC.

Revision Contents
Changeset List

PathSize
services/
blob/
src/
20 lines
30 lines

Diff 18811

View Options

services/blob/src/constants.rs

Loading...
View Options

services/blob/src/service.rs

Loading...