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 the possibility to shuffle NameServers #1920

Merged
merged 35 commits into from Jun 5, 2023

Conversation

Edu4rdSHL
Copy link
Contributor

Hello, this PR makes it possible to shuffle DNS NameServers to avoid overloads to the first configured ones.

The default value for shuffle_dns_servers is set to true, it greatly improves performance in all kinds of tasks so IMO it makes sense to default to it. For comparison:

A lookup for 11500 hosts, having 1000 resolvers, and using 100 threads takes ~10 minutes to complete, with this change it only takes 55 seconds. That means ~10x of performance increase.

It's the continuation of #1632, I have tried implemeting SmallRng, however I haven't had success because it doesn't implement Send and therefore can't be send between threads safely.

It can be improved in the future, but for now I think that it's a good start.

Hello, this PR makes it possible to shuffle DNS NameServers to avoid overloads to the first configured ones.

The default value for `shuffle_dns_servers` is set to true, it greatly improves performance in all kinds of tasks so IMO it makes sense to default to it. For comparison:

A lookup for 11500 hosts, having 1000 resolvers, and using 100 threads takes ~10 minutes to complete, with this change it only takes 55 seconds. That means ~10x of performance increase.

It's the continuation of hickory-dns#1632, I have tried implemeting `SmallRng`, however I haven't had success because it doesn't implement `Send` and therefore can't be send between threads safely.

It can be improved in the future, but for now I think that it's a good start.
@djc djc mentioned this pull request Apr 26, 2023
4 tasks
Copy link
Member

@bluejekyll bluejekyll left a comment

Choose a reason for hiding this comment

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

I like this, nice and simple, adds exactly what I would think of for this feature without too much over head. Thanks for the PR.

crates/resolver/src/config.rs Outdated Show resolved Hide resolved
Copy link
Member

@bluejekyll bluejekyll left a comment

Choose a reason for hiding this comment

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

Thanks for making that change. Looks good to me.

@bluejekyll
Copy link
Member

@djc, I'm going to merge this unless you have concerns.

@bluejekyll
Copy link
Member

oops, I dropped the ball on this. @Edu4rdSHL , do you think you would mind rebasing this?

@Edu4rdSHL
Copy link
Contributor Author

@bluejekyll done.

@bluejekyll bluejekyll merged commit d2f8df8 into hickory-dns:main Jun 5, 2023
17 checks passed
@bluejekyll
Copy link
Member

Sorry for the delay. Thank you for the PR!

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

9 participants