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

[FIX] smaller chunked upload size for slow connections #122

Merged
merged 1 commit into from
Dec 21, 2022

Conversation

chfsx
Copy link
Contributor

@chfsx chfsx commented Dec 14, 2022

This is concerning #88:
Defining the right chunk size is difficult. A larger chunk size leads to faster uploads, but in the case of slow connections, the upload is aborted because, for example, the connection time is exceeded. Therefore, we would probably have to change to a very small chunk size if very slow connections are also to be supported.
Suggestion here:

  • Half upload limit or
  • 2MB
    what is smaller will be used...

@dagraf
Copy link

dagraf commented Dec 15, 2022

I do not fully understand what chunk size you defined. @chfsx Could you please explain shortly what solution you chose for this PR? As a reminder: In the 'old' plugin, the default chunk size was set to 20MB, and we never heard complaints about uploads getting stuck.

@chfsx
Copy link
Contributor Author

chfsx commented Dec 16, 2022

@dagraf with this PR the chunk size is either 2MB or (if the PHP upload-limit is < 4MB) half this upload-limit (since chunkes should be smaller that the php upload-limit. Smaller upload-limits lead to longer upload times but better support for slow connections. It's always a tradeof and there isn't a perfect value IMO.

@chfsx
Copy link
Contributor Author

chfsx commented Dec 16, 2022

But we can go with 20MB since you mentioned there haven't been problems with this value in the past

@DZenker
Copy link

DZenker commented Dec 16, 2022

But,
min($upload_limit / 2, 2000000)
means that the chunk size will ALWAYS be 2 MB only. I think that nobody is using an ILIAS server with an upload limit <= 4 MB (at least in production).
That means 20 MB would be a good compromize.

Currently, we are using a chunk size of 50 MB in our production system with the Fluxlabs version of the plugin (and before 100 MB for some time), and I've never realized issues described in #88.

@chfsx
Copy link
Contributor Author

chfsx commented Dec 16, 2022

@DZenker not ALWAYS but nearly ;-) Absolutely, nobobdy will run ILAIS with an upload-limit of just 3MB, but we must catch the case. I think 20 MB would be a good compromise, too and I'll update the PR accordingly.

@DZenker
Copy link

DZenker commented Dec 16, 2022

I implemented this fix in our test installation - but I've increased the chunk size to 20 MB instead of 2 MB - and can confirm that upload is working now even with slower internet connection. For details, see comment in #88.

@chfsx chfsx force-pushed the fix/88-large-uploads branch from 2f35681 to 44090fb Compare December 21, 2022 08:38
@chfsx
Copy link
Contributor Author

chfsx commented Dec 21, 2022

Thanks a lot for your feedbacks! I updated the PR to use 20MB as a default which seems to be a good compromize.

@okaufman
Copy link
Contributor

Thank you very much for this fix. It looks very good.

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.

4 participants