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

Perform a binary search to find optimal size of DNS responses #4037

Merged

Conversation

pierresouchay
Copy link
Contributor

Will fix #4036

Instead of removing one by one the entries, find the optimal
size using binary search.

For SRV records, with 5k nodes, duration of DNS lookups is
divided by 4 or more.

Results with around 5k nodes on local agent, dev mode:

Before Optimization: SRV~100ms ; A ~25ms
After Optimization: SRV ~20ms ; A ~20ms

Will fix hashicorp#4036

Instead of removing one by one the entries, find the optimal
size using binary search.

For SRV records, with 5k nodes, duration of DNS lookups is
divided by 4 or more.
Copy link
Member

@mkeeler mkeeler left a comment

Choose a reason for hiding this comment

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

@pierresouchay This looks good to me. Thanks for the PR.

@pearkes pearkes added this to the 1.1.0 milestone Apr 20, 2018
agent/dns.go Outdated

resp.Answer = originalAnswser[:median]
if hasExtra {
resp.Extra = originalExtra[:median]
Copy link
Member

Choose a reason for hiding this comment

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

Correct me if you think otherwise but I think lines 733 and 734 here are unnecessary as well as the originalIndex and originalExtra variables.

The hasExtra block should just be a call to syncExtra. slicing originalExtra should not do anything (other than eat a few cpu cycles) as syncExtra will just overwrite resp.Extra with a brand new slice of its making. Also since syncExtra doesn't modify the RR index there shouldn't be a need to hold the originalIndex and reset index to it.

So I would remove those two vars and those extra lines

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mkeeler Yes, you are right, I was confused by a unit test at a given time, but since no modification is performed, it is safe. Done in next patch

@pierresouchay
Copy link
Contributor Author

Damned unstable Travis CI

Copy link
Member

@mkeeler mkeeler left a comment

Choose a reason for hiding this comment

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

@pierresouchay If you are finished with this I will go ahead and merge it.

@pierresouchay
Copy link
Contributor Author

Yes, I am done with it

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.

Improve DNS performance on large clusters
3 participants