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(balancer) update default size from 100 to 10000 #3296

Merged
merged 1 commit into from
Mar 20, 2018

Conversation

Tieske
Copy link
Member

@Tieske Tieske commented Mar 14, 2018

Updates the default balancer size from 100 to 65535 (max size). The original size is to low as the advice is 100 slots for each target.

This should have been included in #3220 since that implemented the performance (speed and footprint) improvement for the balancer to enable the new size default.

Related #3220

@hishamhm
Copy link
Contributor

Should we really go with 64k as a default? It does have a noticeable (even if now smaller) memory impact. I wouldn't imagine upstreams with 655 targets to be a common scenario, but there will br lots of upstreams with a low number of targets out there (or even 1 if the goal is to use healthchecks only). Isn't, say, 10000 more than enough for a default value?

@Tieske Tieske force-pushed the fix/balancer-size branch from 8427fde to 3679d98 Compare March 14, 2018 19:45
@Tieske Tieske changed the title fix(balancer) update default size from 100 to 65535 fix(balancer) update default size from 100 to 10000 Mar 14, 2018
@Tieske
Copy link
Member Author

Tieske commented Mar 14, 2018

updated to 10k slots

@Tieske Tieske force-pushed the fix/balancer-size branch from 3679d98 to 01941cb Compare March 14, 2018 21:31
The original size was way to low to guarantee proper random
distribution beyond  3-4 targets. The recommended size is 100
slots per target. The new default covers up to 100 targets.

The approximate footprint per upstream:
- 65535 slots, 100 targets: 1.6MB
- 10000 slots, 100 targets: 375kB

This fix should have been included in the latest update of the
DNS library dependency.

Related #3220
@Tieske Tieske force-pushed the fix/balancer-size branch from 01941cb to 93d28a6 Compare March 14, 2018 21:33
Copy link
Member

@thibaultcha thibaultcha left a comment

Choose a reason for hiding this comment

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

LGTM, as discussed irl!

@thibaultcha thibaultcha added pr/ready This PR is considered ready and can be merged at anytime (given it received no subsequent changes) and removed pr/please review labels Mar 15, 2018
@thibaultcha thibaultcha merged commit ba10ca5 into next Mar 20, 2018
@thibaultcha thibaultcha deleted the fix/balancer-size branch March 20, 2018 20:40
thibaultcha pushed a commit that referenced this pull request Mar 20, 2018
The original size was way to low to guarantee proper random
distribution beyond  3-4 targets. The recommended size is 100
slots per target. The new default covers up to 100 targets.

The approximate footprint per Upstream:
- 65535 slots, 100 targets: 1.6MB
- 10000 slots, 100 targets: 375kB

This fix should have been included in the latest update of the
DNS library dependency.

Related #3220 
From #3296
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr/ready This PR is considered ready and can be merged at anytime (given it received no subsequent changes)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants