Skip to content

[auth] smarter backend lookup aborts #15450

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

miodvallat
Copy link
Contributor

Short description

In a PR sometime ago, I expressed the wish for a way to abort a record lookup once we have enough information for our current needs and do not need to process the remaining results.

It turns out that such a function exists, it's Ueberbackend::lookupEnd(), and it works by reading the remaining results in a vacuum.

This PR performs three things:

  • it moves part of the lookupEnd logic down one stair to the DNSBackend level, and allows that part to be overridden by backends, should they have faster/better logic for that operation.
  • it adds a few more uses of lookupEnd in pdns_server and pdnsutil.
  • it adds better lookupEnd logic for a few backends. (it might be possible to also do this for the SQL-based backends, at least some of them, but that's probably for a later PR)

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)
  • forgotten that it is generally frowned upon to submit evil PRs on friday

@coveralls
Copy link

coveralls commented Apr 18, 2025

Pull Request Test Coverage Report for Build 16339109770

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 32 of 34 (94.12%) changed or added relevant lines in 7 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall first build on abort_retry_ignore at 65.745%

Changes Missing Coverage Covered Lines Changed/Added Lines %
modules/lmdbbackend/lmdbbackend.cc 4 6 66.67%
Totals Coverage Status
Change from base Build 16339020776: 65.7%
Covered Lines: 127512
Relevant Lines: 165360

💛 - Coveralls

@mind04
Copy link
Contributor

mind04 commented Apr 18, 2025

I wonder how big the performance hit of this pull is. Stopping half way means no caching in the query cache.

@miodvallat
Copy link
Contributor Author

I wonder how big the performance hit of this pull is. Stopping half way means no caching in the query cache.

Allow me to disagree. This is "below" the query cache, and only affects communication with the backend. None of these individual records, which are now skipped, would have ended up in a cache.

@miodvallat miodvallat changed the title [auth] abort lookups [auth] smarter abort backend lookups Apr 18, 2025
@miodvallat miodvallat changed the title [auth] smarter abort backend lookups [auth] smarter backend lookups aborts Apr 18, 2025
@miodvallat miodvallat changed the title [auth] smarter backend lookups aborts [auth] smarter backend lookup aborts Apr 18, 2025
@miodvallat miodvallat force-pushed the abort_retry_ignore branch from af3ec06 to a434eb1 Compare May 28, 2025 07:35
@miodvallat miodvallat force-pushed the abort_retry_ignore branch from a434eb1 to e69b2d8 Compare July 17, 2025 07:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants