Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

api: Implement resumable uploads for VOD #1112

Merged
merged 25 commits into from
Aug 2, 2022
Merged

Conversation

victorges
Copy link
Member

@victorges victorges commented Jun 17, 2022

[DEV] How to test this

  • Go to packages/api folder and run yarn dev
  • Go to packages/tus-test folder, set LP_API_KEY env and run yarn start

Useful links

What's missing

The "only" thing we're really missing now is changing the local filesystem storage to a Google Cloud Storage storage,
which is what we use for VOD today. For that it might be necessary to change directUpload file names again, maybe
losing all the folders and just saving something on the root of the bucket like directUpload-<playbackId> or the other
way around <playbackId>-directUpload just so it has the same prefix as the final asset folder.

It may seem like a simple change, but other problems might happen on the way to switching the storage. Let's just hope
that not too many.

@vercel
Copy link

vercel bot commented Jun 17, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
livepeer-com ❌ Failed (Inspect) Sep 2, 2022 at 7:50PM (UTC)
livepeer-studio ✅ Ready (Inspect) Visit Preview Sep 2, 2022 at 7:50PM (UTC)

async function createTestTusServer() {
const tusTestServer = new tus.Server();
tusTestServer.datastore = new tus.FileStore({
path: "/tmp",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might wanna use os.tmpdir() here instead of the hardcoded /tmp just for compatibility with non-Unix platforms.

Can also consider using fs.mkdtempSync to create a folder inside /tmp just not to pollute devs machines too much. Could be just /tmp/studio-tus for example to keep everything in there. WDYT?

Found all those functions here: https://blog.mastykarz.nl/create-temp-directory-app-node-js/
But I don't think we need to bother deleting the directory after the tests are done. Seems OK to just write to the same folder every time and the dev can clean it up eventually if desired.

async function createTestTusServer() {
const tusTestServer = new tus.Server();
tusTestServer.datastore = new tus.FileStore({
path: "/tmp",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're probably missing a namingFunction here as well. Keep in mind that you can't use the same one as below, since that one creates a file name with a directory, and the FileStore seemed to not like that when I tested it. You can just inline some function here like

Suggested change
path: "/tmp",
path: "/tmp",
namingFunction: (req) => req.res.getHeader("livepeer-playback-id").toString(),

To keep the file name just equal to the playback ID. Then it's also easy on your tests to check if the file was created and is equal to what you sent. WDYT?

@codecov
Copy link

codecov bot commented Jul 29, 2022

Codecov Report

Merging #1112 (f46f71c) into master (752703f) will increase coverage by 0.28819%.
The diff coverage is 55.17241%.

Impacted file tree graph

@@                 Coverage Diff                 @@
##              master       #1112         +/-   ##
===================================================
+ Coverage   50.41836%   50.70655%   +0.28819%     
===================================================
  Files             66          66                 
  Lines           4183        4246         +63     
  Branches         740         748          +8     
===================================================
+ Hits            2109        2153         +44     
- Misses          1825        1836         +11     
- Partials         249         257          +8     
Impacted Files Coverage Δ
packages/api/src/store/errors.ts 68.00000% <0.00000%> (-9.27273%) ⬇️
packages/api/src/test-server.ts 93.02326% <ø> (ø)
packages/api/src/webhooks/cannon.ts 47.36842% <0.00000%> (-0.30600%) ⬇️
packages/api/src/task/scheduler.ts 51.16279% <33.33333%> (-0.64444%) ⬇️
packages/api/src/app-router.ts 51.94805% <40.00000%> (-2.10600%) ⬇️
packages/api/src/controllers/asset.ts 49.64286% <57.14286%> (+5.29502%) ⬆️
packages/api/src/test-helpers.ts 81.31868% <100.00000%> (+1.08611%) ⬆️
packages/api/src/store/table.ts 68.24324% <0.00000%> (+1.35134%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 752703f...f46f71c. Read the comment docs.

@victorges victorges marked this pull request as ready for review August 1, 2022 16:11
@victorges victorges requested a review from a team as a code owner August 1, 2022 16:11
@victorges victorges changed the title [WIP] api: Resumable uploads api: Implement resumable uploads support Aug 1, 2022
@victorges victorges changed the title api: Implement resumable uploads support api: Implement resumable uploads for VOD Aug 1, 2022
Copy link
Member Author

@victorges victorges left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but I opened the PR so I can't approve 😛

The tests need some fixes though, as it's missing a couple of important checks to ensure proper functioning. Both in the "setup test" logic as well as in the test code itself.

packages/api/src/controllers/asset.ts Outdated Show resolved Hide resolved
packages/api/src/controllers/asset.ts Outdated Show resolved Hide resolved
packages/api/src/test-helpers.ts Outdated Show resolved Hide resolved
packages/tus-test/index.html Outdated Show resolved Hide resolved
packages/api/src/webhooks/cannon.ts Outdated Show resolved Hide resolved
packages/api/src/controllers/asset.test.ts Outdated Show resolved Hide resolved
packages/api/src/controllers/asset.test.ts Outdated Show resolved Hide resolved
packages/api/src/controllers/asset.test.ts Outdated Show resolved Hide resolved
packages/api/src/controllers/asset.test.ts Outdated Show resolved Hide resolved
packages/api/src/controllers/asset.test.ts Show resolved Hide resolved
Was missing the tus-js-client dependency and needed to fix the check on
URL format.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants