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

Add throttle support #40

Merged
merged 3 commits into from Oct 30, 2015
Merged

Add throttle support #40

merged 3 commits into from Oct 30, 2015

Conversation

ghost
Copy link

@ghost ghost commented Oct 25, 2015

No description provided.

@dr-dimitru
Copy link
Member

@NROL39 why? I mean what this PR actually fixes/enhances

@ghost
Copy link
Author

ghost commented Oct 25, 2015

It adds throttling support, let's say you have a hundred users, your server has 100mbps bandwidth, at the moment what will happen is that you'll be sending data as fast as possible, choking the other connections (try downloading multiple large files at the same time, you'll notice some will be downloading at a fast rate while some will have much lower rate).

Basically, it's so you can divide resources between multiple clients without having clients interfere with each other's downloads

@dr-dimitru
Copy link
Member

@NROL39 got it, but why not make build in (all pipes by default) with ability to change (bps). Now if user does not passes throttle to Files instance, this feature won't be used

@ghost
Copy link
Author

ghost commented Oct 25, 2015

That's right, I feel this should be an opt-in feature rather than an opt-out (although that is entirely up to you)

@dr-dimitru
Copy link
Member

Better to have it enabled by default. But we need to specify default bps? Which value will be optimal?

@ghost
Copy link
Author

ghost commented Oct 25, 2015

Well, it depends on the server's bandwidth, average file size and concurrent user count, every project will have it's own optimal range which is why i think it should be opt-in.

@dr-dimitru
Copy link
Member

Agree now.
And why this feature is not applied to 206 response?

@ghost
Copy link
Author

ghost commented Oct 25, 2015

There we go, 206 support added.

@dr-dimitru
Copy link
Member

@NROL39 Thank you. I'll run tests, then merge and publish

@dr-dimitru
Copy link
Member

Could you also add check @throttle, Match.oneOf Boolean, Number into constructor and update docs and in-code doc-block?

dr-dimitru added a commit that referenced this pull request Oct 30, 2015
@dr-dimitru dr-dimitru merged commit 1ca9444 into veliovgroup:master Oct 30, 2015
dr-dimitru added a commit that referenced this pull request Oct 30, 2015
 - Fix issue mentioned in #41
 - Add throttle #40
@dr-dimitru dr-dimitru mentioned this pull request Oct 30, 2015
dr-dimitru added a commit that referenced this pull request Oct 30, 2015
v1.3.11
 - Fix issue mentioned in #41
 - Add throttle #40
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

1 participant