[services][blob] Add methods to manage the blob table
ClosedPublic
Actions

Authored by bartek on Nov 20 2022, 11:46 PM.

Details

Reviewers

varun
• jon
tomek
marcin

Commits

rCOMM6ffad5aabef5: [services][blob] Add methods to manage the blob table

Summary

Implemented methods for managing the blob-service-blob table. They will be needed in subsequent diffs.

This is mostly the same approach as in Identity Service, except I use anyhow to handle errors for now, for the sake of simplicity. Proper error handling is going to be introduced later.

Link to the C++ counterpart

Part of ENG-2300

Test Plan

Called these locally and ensured that entities are properly inserted/read/removed from database.

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

bartek created this revision.Nov 20 2022, 11:46 PM

bartek held this revision as a draft.

Herald added subscribers: atul, ashoat. · View Herald TranscriptNov 20 2022, 11:46 PM

bartek added inline comments.Nov 20 2022, 11:57 PM

services/blob/src/database.rs
52 ↗	(On Diff #18627)	Currently, I store it as an `int64` timestamp to keep it compatible with the old C++ implementation. I have prepared a possible commit which implements this field as `chrono::DateTime`, while still storing it as a timestamp string in the database. But maybe it would be a better idea to drop backwards compatibility and use `to_rfc3339()` as it is done in Identity service?
156–164 ↗	(On Diff #18627)	This helper function is copy-pasted from the Identity service (except temporarily using `anyhow` for error handling) It would be a good idea to move some common code to a Rust library shared across services, the same way we have it for C++ code (the `services/lib/src` dir). I can create a Linear task for this unless it has already been created.

Harbormaster completed remote builds in B13598: Diff 18627.Nov 20 2022, 11:59 PM

bartek added a child revision: D5694: [services][blob] Add reverse index database methods.Nov 21 2022, 12:56 AM

bartek added 1 blocking reviewer(s): varun.Nov 21 2022, 12:58 AM

bartek added inline comments.Nov 21 2022, 3:54 AM

services/blob/src/database.rs
156–164 ↗	(On Diff #18627)	I haven't found a task for this so I created one: https://linear.app/comm/issue/ENG-2311/shared-rust-code-across-services

bartek added inline comments.Nov 21 2022, 3:56 AM

services/blob/src/database.rs
52 ↗	(On Diff #18627)	Created a task to track this: https://linear.app/comm/issue/ENG-2309/use-datetime-type-for-created-field-of-blobitem

bartek edited the summary of this revision. (Show Details)Nov 21 2022, 3:57 AM

Did you intentionally leave this diff in Draft state? Just want to make sure it isn't stuck here for some other (possibly CI-related?) reason

bartek published this revision for review.Nov 22 2022, 1:54 AM

ashoat added inline comments.Nov 22 2022, 5:10 AM

services/blob/src/database.rs
52 ↗	(On Diff #18627)	The only reason to keep backwards compatibility is to make testing easier for you. We haven't introduced any usages of this service in production, so we should be okay breaking backwards compatibility if you are okay with it.

varun requested changes to this revision.Nov 22 2022, 10:09 AM

varun added inline comments.

services/blob/src/database.rs
52 ↗	(On Diff #18627)	+1 what Ashoat said, and I think using `rfc3339` is good for consistency
156–164 ↗	(On Diff #18627)	Do we have a task for creating new Error types and moving away from Anyhow?

This revision now requires changes to proceed.Nov 22 2022, 10:09 AM

bartek edited parent revisions, added: D5716: [services][blob] Use chrono::DateTime for timestamps; removed: D5682: [services][blob] Provide S3 and DB to service, add more constants.Nov 24 2022, 1:28 AM

bartek edited the summary of this revision. (Show Details)

Rebase
Use RFC 3339 for created datetime

services/blob/src/database.rs
156–164 ↗	(On Diff #18627)	Do we have a task for creating new Error types and moving away from Anyhow? Do you mean moving away from Anyhow only in Blob service or in general? If the former, then I created ENG-2307, but for the latter case I haven't found any.

Harbormaster completed remote builds in B13726: Diff 18806.Nov 24 2022, 1:45 AM

tomek accepted this revision.Nov 24 2022, 4:45 AM

tomek added inline comments.

services/blob/src/database.rs
166 ↗	(On Diff #18806)	Shouldn't we somehow specify the format from which we're parsing?

varun accepted this revision.Nov 28 2022, 11:04 PM

varun added inline comments.

services/blob/src/database.rs
166 ↗	(On Diff #18806)	The format is implied, actually, but a comment might be helpful https://docs.rs/chrono/latest/chrono/struct.DateTime.html#impl-FromStr-for-DateTime%3CUtc%3E
156–164 ↗	(On Diff #18627)	I meant for the former. Thanks for creating that task!