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

minimal tus implementation #669

Closed
wants to merge 1 commit into from
Closed

Conversation

butonic
Copy link
Contributor

@butonic butonic commented Apr 19, 2020

I'm trying to split up #661 into smaller pieces.

AFAICT this is the minimal useable patch:

  • makes the dataprovider speak tus.io
  • makes the storageprovider return the id created with NewUpload from the tus interface
  • implements tus upload for eos, local and owncloud storage drivers
    • the eos driver still uses a temp file, but it is now assembled using tus
  • uses tus.io upstream handler, not a fork

out of scope

  • tus upload for s3 storage driver
  • the chunking v2 implementation is left out, as it will be inefficient: the clients do not send the destination when creating the upload with MKCOL. So the ocdav handler will have to temporarily store the chunks locally and wait for the final MOVE before it can transfer the upload to the dataprovider.
  • use grpc to stream data directly to eos

known issues

  • downloading a 5GB fails because of the 10sec timeout we currently use in the default httpclient

@PVince81 and @LukasHirt had this running after adding more CORS patches:

@labkode I'd like to get this in and work on reva cli commands to upload files so the patchset does not grow too big.

@butonic butonic requested a review from labkode as a code owner April 19, 2020 12:32
@butonic
Copy link
Contributor Author

butonic commented Apr 19, 2020

Hm, I think I could even leave out the ocdav changes.

@butonic butonic force-pushed the tus-minimal branch 3 times, most recently from 7052e56 to 0afd832 Compare April 19, 2020 19:57
@butonic
Copy link
Contributor Author

butonic commented Apr 19, 2020

@labkode where should the binary data for an upload stream be stored? In the destination folder? In the shadow tree? in a .uploads tree next to the shadow tree? I would not put it in the destination directory, in case you want to trigger a workflow when it has been uploaded. It should be moved to the destination dir.

@PVince81
Copy link
Contributor

should we also set "bigfilechunking" to v3.0 on the capabilities ?
one trouble with that setting is that it doesn't tell whether the other chunking capabilities are enabled or not, might need a new attribute

I'm aware that the TUS capability should be queried on specific endpoints, so I'm wondering if it makes sense to advertise the possibility of TUS at all on a global scope like the capabilities.

@butonic
Copy link
Contributor Author

butonic commented Apr 20, 2020

@PVince81 imo exposing storage capabilities in a global capabilities endpoint no longer makes sense, because there is more than one storage at play. AFAICT storage capabilities in reva should be part of tdhe storage registry. Actually, it depends on what kind of capabilities we want to indicate. The sharing folder may have weird properties: it cannot be deleted or unshared, but you cannot upload folders there as well? Sometimes new folders appear. While all of this can be covered by permissions maybe a capability makes more sense

Regarding chunking, for now I would expose tus as a new capability. It would allow clients to detect if they should even bother with checking the PROPFIND responses of collections for tus-resumable headers. But not as part of this PR, but as part of the ocdav and ocs changes.

Regarding bigfilechunking ... no Idea what this is used for. in the code I commented that I guess it has to do with chunking v1. Android, ios and pyocclient seem to at least read it: https://github.com/search?q=org%3Aowncloud+bigfilechunking&type=Code

@butonic
Copy link
Contributor Author

butonic commented Apr 21, 2020

Todo add tus to upload cmd

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
@butonic
Copy link
Contributor Author

butonic commented Apr 24, 2020

don't bother just go to #674 directly

@butonic butonic closed this Apr 24, 2020
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.

2 participants