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 ECS-based cache entry refresh code #6300

Merged
merged 1 commit into from Feb 26, 2018

Conversation

Projects
None yet
2 participants
@ahupowerdns
Member

ahupowerdns commented Feb 22, 2018

This would lead to us not refreshing ECS-varying answers in the cache, and therefore an even more depressed cache rate.

Thanks to @liordot who found the issue & provided a fix in #6241. This code has been tested by @paddg where it slightly reduced CPU load.

Short description

The fix seems trivial, but a second look is required.

Checklist

I have:

  • read the CONTRIBUTING.md document
  • compiled and tested this code
  • included documentation (including possible behaviour changes)
  • documented the code
  • added or modified regression test(s)
  • added or modified unit test(s)
  • checked that this code was merged to master
Fix ECS-based cache entry refresh code
This would lead to us not refreshing ECS-varying answers in the cache, and therefore an even more depressed cache rate.

Thanks to @liordot who found the issue & provided a fix in #6241.

@ahupowerdns ahupowerdns requested a review from rgacogne Feb 22, 2018

@rgacogne

LGTM. I think we could improve this by not looking into d_ecsIndex and adding the mask if we know it should already be in there, but this fixes a real issue and has been tested, so the improvement might be done in a separate PR at a later time.

}
ecsIndex->addMask(*ednsmask);
/* don't bother building an ecsIndex if we don't have any netmask-specific entries */
if (ednsmask && !ednsmask->empty()) {

This comment has been minimized.

@rgacogne

rgacogne Feb 23, 2018

Member

Since the only case where we might have an existing entry in d_cache for this exact (qname, qtype, ednsmask) tuple, but not one in d_ecsIndex, is that the entry has expired and was removed from d_ecsIndex but it's still waiting for garbage collection in d_cache, we could skip the lookup into d_ecsIndex and the call to ecsIndex->addMask() unless isNew or stored->d_ttd <= now. It would require a comment explaining why, though.

@rgacogne rgacogne merged commit 32a2909 into PowerDNS:master Feb 26, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@rgacogne rgacogne added this to the rec-4.1.x milestone Feb 26, 2018

pieterlexis added a commit to pieterlexis/pdns that referenced this pull request Mar 26, 2018

@pieterlexis pieterlexis referenced this pull request Mar 26, 2018

Merged

Recursor 4.1.2 backports #6387

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