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

Make ConcurrencyLimiter's initialLimit configurable #1077

Open
abless opened this issue Apr 23, 2019 · 6 comments
Open

Make ConcurrencyLimiter's initialLimit configurable #1077

abless opened this issue Apr 23, 2019 · 6 comments

Comments

@abless
Copy link
Contributor

abless commented Apr 23, 2019

AIMDLimit uses an initialLimit of 10, which seems really low and overly conservative. Instead of turning off limiting completely, it'd be preferable to start with a more reasonable, configurable limit.

@abless
Copy link
Contributor Author

abless commented Apr 23, 2019

For more context, the current model performs poorly at startup for services that perform short bursts of requests. Very early on in the service's lifecycle, these short bursts will get heavily penalized since the window is so small and gets only additively incremented.

@markelliot
Copy link
Contributor

I think we'd prefer to adjust the number rather than make it configurable.

@j-baker
Copy link
Contributor

j-baker commented Apr 23, 2019

out of interest, what makes this be a pain? (happy to be sent an internal link!)

@abless
Copy link
Contributor Author

abless commented Apr 23, 2019

We have a service that, at startup, makes a burst of requests to an underlying storage service. The results of these requests are stored in a cache so it's only bursty at startup, when the cache is empty. What we've noticed is that the slow_acquire_p99 goes up to ~30s, even though we're not receiving any 503/429 from the storage service. My only explanation, then, is that we're just limited by the small number of "tokens" at startup.

When using the no-op limiter, this results in a significant perf increase without dropped requests or throttling. However, disabling the concurrency limiter completely seems not ideal - I'd rather be able to configure saner default for our use case, or adjust them globally.

@stale
Copy link

stale bot commented Sep 23, 2019

This issue has been automatically marked as stale because it has not been touched in the last 60 days. Please comment if you'd like to keep it open, otherwise it'll be closed in 7 days time.

@stale stale bot added the stale label Sep 23, 2019
@iamdanfox iamdanfox removed the stale label Sep 23, 2019
@JacekLach
Copy link
Contributor

👍 - while I agree that just bumping the limit is probably best, it seems people are wary to do this globally in case things break. Having a opt-in flag that lets you to move to a new limit base (and roll back via config if things break) seems a best way forward

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

No branches or pull requests

5 participants