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

Geoip #6585

Merged
merged 6 commits into from May 14, 2018

Conversation

Projects
None yet
2 participants
@cmouse
Contributor

cmouse commented May 8, 2018

Short description

Fixes #6584, and does few small code quality improvements.

Includes an optimization to make lookups faster.

Checklist

I have:

  • read the CONTRIBUTING.md document
  • compiled this code
  • tested this code
  • included documentation (including possible behaviour changes)
  • documented the code
  • added or modified regression test(s)
  • added or modified unit test(s)

The change here is mainly that we try and avoid returning netmask of 0 for any question to avoid poisoning cache.

cmouse added some commits May 8, 2018

geoipbackend: Make sure initial netmask is not too wide
Otherwise cache will be "poisoned" with too wide value, rendering
the geoip functionality useless.

Fixes #6584, found by @blop
geoipbackend: Optimize lookup
Avoids expensive copying operations
@pieterlexis

This comment has been minimized.

Member

pieterlexis commented May 11, 2018

The last two commits work for me.

Testing 16K netmasks as services, before this PR, my CPUs would be overloaded at 400 QPS, after this commit, 40000 QPS raise CPU usage to ~20%.

@pieterlexis pieterlexis merged commit 156e381 into PowerDNS:master May 14, 2018

4 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
lgtm analysis: C/C++ No alert changes
Details
lgtm analysis: JavaScript No alert changes
Details
lgtm analysis: Python No alert changes
Details

@cmouse cmouse referenced this pull request May 15, 2018

Merged

Geo 4.1.2 #6608

4 of 8 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment