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

Small improvements to reverse resolver #451

Merged
merged 1 commit into from
Sep 7, 2015
Merged

Conversation

tomwilkie
Copy link
Contributor

Don't rate limit requests that have already been resolved, and cache negative responses.

We do this as we generate so many reverse-ip lookups that we quickly overwhelm the the channel and start dropping them. At a rate of 10 lookups per second, we never get through the queue.

By not rate limiting requests that are in the cache we don't actually do any more requests, it just allows us to burn through the queue quicker. And by caching negative responses, we prevent the queue for growing massive to begin with.

I tested this by doing a telnet google.com 80 and then checking we got the right hostname in the applications view. Before this fix it would never show any hostname, after the fix it shows something.

@tomwilkie tomwilkie changed the title Don't rate limit requests that have already been resolved, and cache … Small improvements to reverse resolver Sep 7, 2015
@tomwilkie tomwilkie force-pushed the reverse-resolver-fix branch from 3297a96 to f5d6872 Compare September 7, 2015 10:30
if _, err := r.cache.Get(request); err == nil {
continue
}
<-r.Throttle // rate limit our DNS resolutions

This comment was marked as abuse.

@inercia inercia assigned inercia and tomwilkie and unassigned inercia Sep 7, 2015
@inercia
Copy link
Contributor

inercia commented Sep 7, 2015

LGTM.

tomwilkie added a commit that referenced this pull request Sep 7, 2015
Small improvements to reverse resolver
@tomwilkie tomwilkie merged commit 9ab98ba into master Sep 7, 2015
@tomwilkie tomwilkie deleted the reverse-resolver-fix branch October 5, 2015 10:07
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.

2 participants