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

limit-rate.d: this is average over several seconds #7970

Closed
wants to merge 1 commit into from

Conversation

@bagder
Copy link
Member

@bagder bagder commented Nov 6, 2021

No description provided.

@@ -17,6 +17,9 @@ Appending 'k' or 'K' will count the number as kilobytes, 'm' or 'M' makes it
megabytes, while 'g' or 'G' makes it gigabytes. The suffixes (k, M, G, T, P)
are 1024 based. For example 1k is 1024. Examples: 200K, 3m and 1G.
The rate limiting logic works on averaging the transfer speed to no more than
the set threshold over a period of multiple seconds.
Copy link
Member

@jay jay Nov 6, 2021

Choose a reason for hiding this comment

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

Ref: #7965

If the goal here is informing the user that rate limiting split second transfers has no effect then I think you should say that, such as "Rate limiting is averaged over several seconds. For shorter transfers it will have no effect."

Loading

Copy link
Member Author

@bagder bagder Nov 6, 2021

Choose a reason for hiding this comment

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

Shorter than what? And it will/might have some effect depending on factors. I'm vague to avoid having to make specific claims that I can't back up.

Loading

Copy link
Member

@jay jay Nov 7, 2021

Choose a reason for hiding this comment

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

If it might have some effect then disregard.

Loading

Copy link

@wnrph wnrph Nov 7, 2021

Choose a reason for hiding this comment

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

This information appears to be outdated. At least the send rate is already limited immediately after sending the header, not only after "multiple" seconds. What does that even mean? After a couple of seconds using the entire bandwidth? After more than two, but undefined number of seconds at an undefined rate?

Loading

Copy link

@wnrph wnrph Nov 7, 2021

Choose a reason for hiding this comment

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

The limiting logic also appears to have been changed such that it is no longer based on averaging (see 4b86113, #971). What's more, before said change, the documentation used to contain a hint along the lines of what is proposed here:

The given rate is the average speed counted during the entire transfer. It
means that curl might use higher transfer speeds in short bursts, but over
time it uses no more than the given rate.

I suppose it was removed for a reason (because it no longer reflects how the speed limit logic works?)

Loading

Copy link
Member Author

@bagder bagder Nov 7, 2021

Choose a reason for hiding this comment

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

I maintain my added sentence is still correct and might help users understand what to expect from this option.

Loading

@bagder bagder closed this in f03778f Nov 8, 2021
@bagder bagder deleted the bagder/docs-limit-rate branch Nov 8, 2021
The rate limiting logic works on averaging the transfer speed to no more than
the set threshold over a period of multiple seconds.
Copy link

@wnrph wnrph Nov 9, 2021

Choose a reason for hiding this comment

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

@fabiankeil What are your thoughts? Is this an accurate description of how the rate limiting works?

Loading

@fabiankeil
Copy link
Contributor

@fabiankeil fabiankeil commented Nov 11, 2021

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants