Page MenuHomePhabricator

[lib] Add useMediaMetadataUpload hook
ClosedPublic

Authored by bartek on Sep 27 2024, 6:59 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Nov 1, 4:46 PM
Unknown Object (File)
Fri, Nov 1, 4:46 PM
Unknown Object (File)
Fri, Nov 1, 4:22 PM
Unknown Object (File)
Fri, Nov 1, 5:35 AM
Unknown Object (File)
Mon, Oct 28, 8:56 AM
Unknown Object (File)
Tue, Oct 22, 5:34 AM
Unknown Object (File)
Tue, Oct 22, 5:33 AM
Unknown Object (File)
Mon, Oct 21, 6:01 AM
Subscribers

Details

Summary

ENG-9350.

Very similiar to the blob upload action, but it was tough to extract some shared code easily.

Test Plan

Tested together with the next diff

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

bartek held this revision as a draft.

Was this meant to be published?

bartek published this revision for review.Sep 28 2024, 9:49 AM
ashoat added inline comments.
lib/actions/upload-actions.js
355

Nit: why not destructure blobHash here too?

387

Why do we say "optionally" here? Seems like we always do it?

This revision is now accepted and ready to land.Sep 30 2024, 4:41 AM
ashoat requested changes to this revision.Sep 30 2024, 4:46 AM

Actually, something I'm confused about...

lib/actions/upload-actions.js
361–385

I'm confused why we need the client to assign a holder here. Shouldn't the keyserver own the holder? Why can't the keyserver do this call?

This revision now requires changes to proceed.Sep 30 2024, 4:46 AM
lib/actions/upload-actions.js
361–385

It was done this way earlier in the blobServiceUpload above in this file. We have the following flow:

  1. Create a holder and see if data_exists
  2. If data doesn't exist, perform blob upload
  3. Only for successful blob uploads let keyserver know by calling the upload_media_metadata endpoint. The endpoint expects a holder to already exist

It was designed this way from the beginning to be more error-proof:
If 1 or 2 fails, the keyserver won't receive any notice, and Blob Service will do the cleanup if needed, and we can retry. If the keyserver did the holder assignment, we could end up with keyserver DB entries without blobs successfully uploaded, and recovering from this isn't that easy.

Here in this function, we do only 1. and 3.

387

I copied the comment from blobServiceUpload above and forgot to update it.

lib/actions/upload-actions.js
361–385

I'm more confused now unfortunately...

Questions:

  1. It seems like in order to understand your response, I need to understand what happens before mediaMetadataUpload is called. Can you share that context?
  2. It sounds like by the time mediaMetadataUpload is called, the blob has already been uploaded? Is that why we're skipping step 2 in this flow?
  3. What's the purpose of setting a new holder here if the blob has already been uploaded?
  4. What do we do with the original holder? Do we clear it?
  5. Why isn't the keyserver owning the holder?
lib/actions/upload-actions.js
361–385
  1. mediaMetadataUpload is called only for already-uploaded thick thread media, which we're migrating into thin thread media.
  2. Yes, it is. It already has a holder (1 and 2 done), but it is a local holder from holder store, and we need another one for keyserver.
  3. I guess I answered in 2.
  4. we discussed it on Linear and agreed to clear it. I'm going to do it in the next diff (where we can take advantage of batching this operation)
  5. This new holder is going to be owned by the keyserver and forgotten locally
ashoat added inline comments.
lib/actions/upload-actions.js
348

Can we rename this function to be more clear?

  • reassignThickThreadMediaForThinThread
  • migrateThickThreadMediaToThinThread

Open to other suggestions as well!

361–385

Thanks for explaining!

This revision is now accepted and ready to land.Sep 30 2024, 6:40 AM
  • Renamed the function
  • Updated comment
  • Slightly improved filename handling
lib/actions/upload-actions.js
348

The first name looks reasonable!

355

The blobHash variable name is taken in L358 for processed input.

This revision was automatically updated to reflect the committed changes.