Page MenuHomePhabricator

[backup] Introduce upload endpoint
ClosedPublic

Authored by michal on Aug 28 2023, 1:33 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, May 4, 3:08 PM
Unknown Object (File)
Sat, May 4, 3:08 PM
Unknown Object (File)
Sat, May 4, 3:08 PM
Unknown Object (File)
Sat, May 4, 3:06 PM
Unknown Object (File)
Sat, May 4, 2:38 PM
Unknown Object (File)
Wed, May 1, 6:50 PM
Unknown Object (File)
Tue, Apr 30, 8:15 AM
Unknown Object (File)
Sun, Apr 28, 3:20 AM
Subscribers

Details

Summary

ENG-4501

Endpoint for uploading the backups.

Depends on D8962

Test Plan

Make a request:

POST http://127.0.0.1:50052/backups
Content-Type: multipart/form-data; boundary=bound
Authorization: Bearer eyJ1c2VySUQiOiAiMSIsICJhY2Nlc3NUb2tlbiI6ICIyIiwgImRldmljZUlEIjogIjMifQ==

--bound
Content-Disposition: form-data; name="backup_id"

id7
--bound
Content-Disposition: form-data; name="user_keys_hash"

keys_hash7
--bound
Content-Disposition: form-data; name="user_keys"
Content-Type: application/octet-stream

keys7
--bound
Content-Disposition: form-data; name="user_data_hash"

data_hash7
--bound
Content-Disposition: form-data; name="user_data"
Content-Type: application/octet-stream

data7
--bound
Content-Disposition: form-data; name="attachments"

a7-1
a7-2
a7-3
--bound--
  • checked that there is a new backup row in dynamodb
  • checked that there are 2 blobs in dynamodb

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Note that blob cleanup (if error occurs later during upload) will be added in another diff.

services/backup/src/http/handlers/backup.rs
34–37 ↗(On Diff #30421)

This part is repeating frequently. Wondering if worth doing a macro:
https://github.com/CommE2E/comm/blob/master/services/blob/src/http/utils.rs

120 ↗(On Diff #30421)
135 ↗(On Diff #30421)

typo

160 ↗(On Diff #30421)

Do logs from receive_promise and send_promise appear inside the forward_to_blob span?
You might need to wrap them in .in_current_span() from tracing-futures crate

services/backup/src/http/handlers/backup.rs
34–37 ↗(On Diff #30421)

If anything I think we could add a wrapper around get_text_field like get_named_text_field that would do the check and return an error if it's not correct. I'm not really a fan of the macro approach.

160 ↗(On Diff #30421)

Seem to work fine:

2023-08-28T13:39:33.689100Z  INFO upload_backup: backup::http::handlers::backup: Upload backup request
2023-08-28T13:39:33.689312Z TRACE upload_backup{backup_id="id7"}:forward_to_blob: backup::http::handlers::backup: Reading blob fields: "user_keys_hash", "user_keys"
2023-08-28T13:39:33.689398Z TRACE upload_backup{backup_id="id7"}:forward_to_blob: backup::http::handlers::backup: Receiving blob data
2023-08-28T13:39:33.689425Z TRACE upload_backup{backup_id="id7"}:forward_to_blob: backup::http::handlers::backup: Finished receiving blob data
2023-08-28T13:39:33.735435Z TRACE upload_backup{backup_id="id7"}:forward_to_blob: backup::http::handlers::backup: Reading blob fields: "user_data_hash", "user_data"
2023-08-28T13:39:33.735567Z TRACE upload_backup{backup_id="id7"}:forward_to_blob: backup::http::handlers::backup: Receiving blob data
2023-08-28T13:39:33.735589Z TRACE upload_backup{backup_id="id7"}:forward_to_blob: backup::http::handlers::backup: Finished receiving blob data

it might be because they are not executed on separate tasks, but they are run on the current task.

bartek added inline comments.
services/backup/src/http/handlers/backup.rs
34–37 ↗(On Diff #30421)

ok, up to you. I suggested a macro because I like complicating things 😉

160 ↗(On Diff #30421)

it might be because they are not executed on separate tasks, but they are run on the current task.

Yeah, that's likely. I wasn't sure what tokio::try_join is doing underneath

This revision is now accepted and ready to land.Aug 28 2023, 6:49 AM

Rebase, fix typos, introduce get_named_text_field function