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

Resumable and chunked track upload #1484

Merged
merged 20 commits into from May 14, 2021
Merged

Resumable and chunked track upload #1484

merged 20 commits into from May 14, 2021

Conversation

vicky-g
Copy link
Contributor

@vicky-g vicky-g commented May 11, 2021

Description

What is the purpose of this PR? What is the current behavior? New behavior? Relevant links (e.g. Trello) and/or information pertaining to PR?

Context: Currently, when a user uploads a track, the entire file is uploaded. In conjunction with a bad network, the user will have to trouble uploading the track. This can cause frustration.

Purpose: This PR is to implement track upload chunking and resumable upload. More details are listed in the upload track design doc

In this PR:

  • abstracting file filter check in fileManager.js to two methods called checkFileType() and checkFileSize()
  • creating a middleware called checkFileMiddleware() that uses the ^ methods
  • rename /track_content_status route to /task_status
  • add a route called /track_content_upload for chunking and resumable upload logic
  • pass new flags useTrackContentPolling and useResumableTrackUpload into libs CN instance
  • refactor uploadTrackAudio() to handle 3 different scenarios:
    • resumable upload + polling track transcode response
    • uploading the entire file + polling track transcode response
    • uploading the entire file + waiting for the track transcode response (original flow)
  • add tus-client-js logic into libs for chunking and resumable upload in method startResumableUpload()

A high level flow chart of upload flow:
IMG_D5042EEEDC09-1

TODO:

  • Address any comments on PR review
  • Fix unit tests
  • Merge

Tests

List the manual tests and repro instructions to verify that this PR works as anticipated. Include log analysis if possible.
❗ If this change impacts clients, make sure that you have tested the clients ❗

  • Track upload works with flag conditions

    • resumable = T, polling = T
    • resumable = T, polling = F (same behavior as ^)
    • resumable = F, polling = F (OG)
    • resumable = F, poling = T
  • Track upload succeeds with 3MB track

  • Track upload succeeds with 200MB track

    • single upload works
    • refresh + upload again resumes from where it was left off at
  • Track upload succeeds with multiple tracks (album)

  • Track upload succeeds with uploads on multiple tabs

    • album, short track, long track on 3 different tabs
  • Track upload succeeds with fast network

    • big track
    • small track
  • Track upload succeeds with slow network (3G slow on console)

    • big track
    • small track
  • POST

    • Dapp handles track content upload middleware failing

      • dapp ugly stack trace page shows with POST route failing
      {"error":"i failed in middleware"}
    • Dapp handles track content upload logic before tus middleware failing

      • dapp ugly stack trace page shows with POST route failing 6x
      {"error":"Error: i failed in creating directories"}
    • Dapp handles track content upload logic during tus middleware failing

      • dapp ugly stack trace page shows with POST route failing 6x
      Something went wrong with that request
      ENOENT: no such file or directory, open 'file_storage/files/tmp_track_artifacts/0cfe7b7d-a5d6-46a9-a2cc-98d36e2ec2db/d402b1a8-e263-47b4-9a78-9dae0655a4b1.mp3'
  • HEAD/PATCH

    • Dapp handles tmp file dir route middleware failing

      • dapp shows heavy load page then nasty stack page
      Server returned error: [423] [Cannot change state of wallet 0xcddd53a34b2f06dac19b59f4187880f0d91d30dc. Node sync currently in progress.] for request: [7e615cd0-0c3f-4956-94fb-a2acda892f93, 7e615cd0-0c3f-4956-94fb-a2acda892f93]
    • Dapp handles tmp file dir route before tus middleware failing

      • dapp retries the patch and head a few times (progress bar kinda goes brazzy; back and forth progress)
      • dapp shows heavy load page then nasty stack page
      {"error":"Error: i failed before the tus logic patch"} 
    • Dapp handles tmp file dir route during tus middleware failing

      • manually deleted the file
      PATCH 410 GONE 
      The file for this url no longer exists

@vicky-g vicky-g changed the title Resumable upload Resumable and chunked track upload May 11, 2021
Copy link
Contributor

@dmanjunath dmanjunath left a comment

Choose a reason for hiding this comment

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

High level this looks great! definitely do want to dig into how the PATCH/HEAD route works a bit more, don't think i fully understand that yet. but otherwise this is looking pretty good

creator-node/src/fileManager.js Outdated Show resolved Hide resolved
creator-node/src/fileManager.js Outdated Show resolved Hide resolved
creator-node/src/routes/files.js Outdated Show resolved Hide resolved
creator-node/src/routes/tracks.js Outdated Show resolved Hide resolved
creator-node/src/routes/tracks.js Outdated Show resolved Hide resolved
creator-node/src/routes/tracks.js Outdated Show resolved Hide resolved
libs/src/services/creatorNode/index.js Outdated Show resolved Hide resolved
creator-node/src/routes/tracks.js Outdated Show resolved Hide resolved
creator-node/src/routes/tracks.js Outdated Show resolved Hide resolved
creator-node/src/routes/tracks.js Outdated Show resolved Hide resolved
creator-node/src/fileManager.js Outdated Show resolved Hide resolved
creator-node/src/fileManager.js Outdated Show resolved Hide resolved
creator-node/src/routes/tracks.js Show resolved Hide resolved
creator-node/src/routes/tracks.js Outdated Show resolved Hide resolved
libs/src/services/creatorNode/index.js Show resolved Hide resolved
libs/src/services/creatorNode/index.js Outdated Show resolved Hide resolved
libs/src/services/creatorNode/index.js Show resolved Hide resolved
libs/src/services/creatorNode/index.js Outdated Show resolved Hide resolved
const { wait } = require('../../utils')
const uuid = require('../../utils/uuid')
const SchemaValidator = require('../schemaValidator')

const CHUNK_SIZE = 2000000 // 2MB
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dmanjunath @raymondjacobson this is the chunking size I decided to use. am open to changing this if either of you think there is a better chunking size. for large tracks, this chunking size might be too small

Copy link
Contributor

Choose a reason for hiding this comment

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

i think this seems reasonable. definitely shouldn't be less than 1MB or more than 2 or 3 so i think this feels right to begin with

Copy link
Member

Choose a reason for hiding this comment

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

am interested in having this in remove config at some point

Copy link
Member

@raymondjacobson raymondjacobson left a comment

Choose a reason for hiding this comment

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

mostly looks good to me at this point! will want to very thoroughly QA on staging, but i love this!

creator-node/src/fileManager.js Outdated Show resolved Hide resolved
creator-node/src/routes/files.js Outdated Show resolved Hide resolved
creator-node/src/routes/tracks.js Show resolved Hide resolved
creator-node/src/routes/tracks.js Show resolved Hide resolved
creator-node/src/routes/tracks.js Outdated Show resolved Hide resolved
@raymondjacobson
Copy link
Member

can we fix the tests?

@vicky-g
Copy link
Contributor Author

vicky-g commented May 13, 2021

can we fix the tests?

yup, coming up

dmanjunath
dmanjunath previously approved these changes May 13, 2021
Copy link
Contributor

@dmanjunath dmanjunath left a comment

Choose a reason for hiding this comment

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

Great work. This looks amazing

@vicky-g vicky-g dismissed stale reviews from dmanjunath and raymondjacobson via d33c02f May 13, 2021 20:16
Copy link
Contributor

@dmanjunath dmanjunath left a comment

Choose a reason for hiding this comment

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

I am so excited for this!!!

@vicky-g vicky-g merged commit 3b26f42 into master May 14, 2021
@vicky-g vicky-g deleted the vg-resumable-upload branch May 14, 2021 18:41
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

3 participants