feat: add payload offloading to GCS read/write paths#2407
Conversation
pjenvey
force-pushed
the
feat/gcs-offload-STOR-579
branch
2 times, most recently
from
c708557 to
e946ec3
Compare
pjenvey
force-pushed
the
feat/gcs-offload-STOR-579
branch
from
e946ec3 to
081276c
Compare
chenba
left a comment
chenba
left a comment
There was a problem hiding this comment.
r+wc
👍 Patch looks good. I have a question about data loss on BSO. Maybe I missed something or it can be addressed separately.
thanks barry
I'll note that Barry's question pointed out a bug on updates which 920588e fixed |
| handle_not_found(db.get_bsos(params).await).map_err(Into::into) | ||
| }, | ||
| async |mut bsos: Paginated<results::GetBso>| { | ||
| for bso in bsos.items.iter_mut() { |
There was a problem hiding this comment.
Maybe not an issue, but If any single download_payload error hits, wouldn't the whole get_collection?full=1 response return 500, even though the DB read succeeded? Acceptable for v1, where one bad/missing object takes down the entire collection fetch.
There was a problem hiding this comment.
That's right -- I don't think there's any alternative? If we can't fulfill the request it should fail (we can't partially give back results e.g.).
| user_id.fxa_uid, | ||
| collection, | ||
| bso_id, | ||
| Uuid::new_v4().hyphenated() |
There was a problem hiding this comment.
Just gonna take note that we're planning on applying a UUID suffix at the end here? So for every new upload/PUT, there's a unique UUID? Reasonable for it making things unique, but I know we were thinking about how exactly to manage the lifecycle rules and this might factor into that decision. Necessary?
There was a problem hiding this comment.
It needs to be unique as the UPDATE path adds a new GCS object -- there could be multiple GCS objects for a BSO at any time (but of course only one canonical one Spanner's payload_link). I'm totally not attached to this specific filename format otherwise.
3 participants
Description
via a new payload_link column (currently Spanner only)
Issue(s)
Closes STOR-578
Closes STOR-579