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 API performance: one DNSSECKeeper per request #4944

Merged
merged 2 commits into from Feb 10, 2017

Conversation

Projects
None yet
4 participants
@wojas
Member

wojas commented Jan 26, 2017

Short description

getZoneInfo() was instantiating a new DNSSECKeeper for every call. Since
DNSSECKeeper opens a new database connection, this resulted in severe
performance issues in /api/v1/servers/localhost/zones with many zones.

This is fixed by passing a DNSSECKeeper instance to getZoneInfo().
With 10,000 zones and the sqlite3 backend, this reduces the API call
execution time from ~2s to ~200ms (from ~7s to 700ms for first call).

Checklist

I have:

  • read the CONTRIBUTING.md document
  • compiled and tested this code
  • included documentation (including possible behaviour changes)
  • documented the code
  • added regression tests
  • added unit tests
  • checked that this code was merged to master
Fix API performance: one DNSSECKeeper per request
getZoneInfo() was instantiating a new DNSSECKeeper for every call. Since
DNSSECKeeper opens a new database connection, this resulted in severe
performance issues in /api/v1/servers/localhost/zones with many zones.

This is fixed by passing a DNSSECKeeper instance to getZoneInfo().
With 10,000 zones and the sqlite3 backend, this reduces the API call
execution time from ~2s to ~200ms (from ~7s to 700ms for first call).
@zeha

This comment has been minimized.

Show comment
Hide comment
@zeha

zeha Jan 27, 2017

Collaborator

As said elsewhere, I think the dnssec flag can just go away, and with it goes the need for DNSSECKeeper in listZones.

Collaborator

zeha commented Jan 27, 2017

As said elsewhere, I think the dnssec flag can just go away, and with it goes the need for DNSSECKeeper in listZones.

@wojas

This comment has been minimized.

Show comment
Hide comment
@wojas

wojas Jan 27, 2017

Member

@zeha The flag is currently being used. Removing it and requiring applications that need it for whatever reason to do individual calls for every single zone would not improve overal performance of the API.

How about adding an ?extra=... GET parameter to this API call and only returning the dnssec field if you pass ?extra=dnssec? This mechanism can also be easily extended to add other fields that the calling application might need (?extra=dnssec,foo,bar). The call takes slightly longer when the extra info is needed, but can save many individual requests that would otherwise be required. Users that don't need the extra fields simply do not provide it.

Such a change would be breaking for applications that use this field, but they can easily be fixed by adding the extra param. Adding this param to an application would also be backward compatible, as auth 4.0 will simply ignore it and always return the field.

Member

wojas commented Jan 27, 2017

@zeha The flag is currently being used. Removing it and requiring applications that need it for whatever reason to do individual calls for every single zone would not improve overal performance of the API.

How about adding an ?extra=... GET parameter to this API call and only returning the dnssec field if you pass ?extra=dnssec? This mechanism can also be easily extended to add other fields that the calling application might need (?extra=dnssec,foo,bar). The call takes slightly longer when the extra info is needed, but can save many individual requests that would otherwise be required. Users that don't need the extra fields simply do not provide it.

Such a change would be breaking for applications that use this field, but they can easily be fixed by adding the extra param. Adding this param to an application would also be backward compatible, as auth 4.0 will simply ignore it and always return the field.

@wojas

This comment has been minimized.

Show comment
Hide comment
@wojas

wojas Jan 27, 2017

Member

One way to keep it backward compatible is to default to?extra=dnssec if the extra param is not set. To to a light query you could pass an empty ?extra=.

Based on my personal quick testing removing the field improves performance by 10-15% after this patch. This is significant enough to provide a way to not incur the overhead, but not to completely remove the functionality.

Member

wojas commented Jan 27, 2017

One way to keep it backward compatible is to default to?extra=dnssec if the extra param is not set. To to a light query you could pass an empty ?extra=.

Based on my personal quick testing removing the field improves performance by 10-15% after this patch. This is significant enough to provide a way to not incur the overhead, but not to completely remove the functionality.

@zeha

Sugg. change (leads to less DB connections), general approach LGTM

Show outdated Hide outdated pdns/ws-auth.cc
@wojas

This comment has been minimized.

Show comment
Hide comment
@wojas

wojas Jan 31, 2017

Member

@zeha I made the suggested change to pass UeberBackend to DNSSECKeeper, thanks!

Member

wojas commented Jan 31, 2017

@zeha I made the suggested change to pass UeberBackend to DNSSECKeeper, thanks!

@zeha

zeha approved these changes Jan 31, 2017

@ahupowerdns ahupowerdns merged commit 9c54783 into PowerDNS:master Feb 10, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment