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 dataset upload #4860

Merged
merged 17 commits into from Oct 20, 2020
Merged

resumable dataset upload #4860

merged 17 commits into from Oct 20, 2020

Conversation

youri-k
Copy link
Contributor

@youri-k youri-k commented Oct 8, 2020

URL of deployed dev instance (used for testing):

  • https://___.webknossos.xyz

Steps to test:

  • upload dataset => should work

Issues:


@youri-k
Copy link
Contributor Author

youri-k commented Oct 8, 2020

ToDo:

  • check how unique resumable uniqueIdentifieris
  • synchronize map accesses

@youri-k youri-k changed the title [WIP] resumable dataset upload resumable dataset upload Oct 15, 2020
@youri-k youri-k marked this pull request as ready for review October 15, 2020 09:12
@youri-k youri-k requested review from fm3 and philippotto and removed request for fm3 October 15, 2020 09:12
@youri-k
Copy link
Contributor Author

youri-k commented Oct 15, 2020

Regarding this ToDo:

check how unique resumable uniqueIdentifier is

resumable uses this code to generate it:

var relativePath = file.webkitRelativePath||file.relativePath||file.fileName||file.name;
var size = file.size;
return(size + '-' + relativePath.replace(/[^0-9a-zA-Z_-]/img, ''));

In my opinion, this isn't unique enough, but I am not sure if it would be enough to add a time code or if we should add the user token in the backend, but that could break it, if the token changes while uploading.

Any ideas @philippotto, @fm3 ?

Copy link
Member

@philippotto philippotto left a comment

Choose a reason for hiding this comment

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

Awesome stuff! Just left a few small comments and you probably want to change the unique identifier thing, right?

youri-k and others added 4 commits October 15, 2020 14:09
Co-authored-by: Philipp Otto <philippotto@users.noreply.github.com>
Co-authored-by: Philipp Otto <philippotto@users.noreply.github.com>
Copy link
Member

@philippotto philippotto left a comment

Choose a reason for hiding this comment

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

Looks good from my side 🕺

Copy link
Member

@fm3 fm3 left a comment

Choose a reason for hiding this comment

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

I like the approach and separating uploading of chunks and a final finish request seems fair. To be honest I had some troubles understanding some of the semantics of pendingUploadChunks and needsWrite in the DataSourceService. Maybe we can figure something out for that (or maybe I just need a verbal explanation)

Copy link
Member

@fm3 fm3 left a comment

Choose a reason for hiding this comment

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

Thanks, with the renamings everything gets clear 👍 Works for me and Backend LGTM

@philippotto
Copy link
Member

🕺 🎉

@fm3 fm3 deleted the resumable-upload branch October 21, 2020 08:28
philippotto added a commit that referenced this pull request Oct 22, 2020
philippotto added a commit that referenced this pull request Oct 22, 2020
fm3 added a commit that referenced this pull request Oct 23, 2020
fm3 added a commit that referenced this pull request Oct 26, 2020
* Revert "Revert "resumable dataset upload (#4860)" (#4887)"

This reverts commit 91f813d.

* do not instantiate DataSourceService outside of datastore module
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make dataset upload robust
3 participants