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

Respect TTL #32

Merged
merged 6 commits into from
Jun 29, 2019
Merged

Respect TTL #32

merged 6 commits into from
Jun 29, 2019

Conversation

bookmoons
Copy link
Contributor

@bookmoons bookmoons commented Jun 26, 2019

Adds expiration support to the cache. Sets up RRs to expire based on TTL.

If the patch is accepted I intend to make a submission to k6 using dnsr as the caching resolver, discussed in grafana/k6#726.

The GooglePTR test seems to be timing out on my system but the expiration tests pass.

Closes #31.

@bookmoons
Copy link
Contributor Author

Cache get() ended up with some nesting. Went for minimal call overhead but it does read awkward. If you think it's too messy I can factor it.

When expiration is enabled, TTL is stored in the RR. The only use is in stringification to zone file format. If that's not worth storing it I can take it out. The current code is stringifying with a static TTL of 3600.

I'm not familiar with the godoc system. Should I be doing anything to make those docs update?

This was referenced Jun 26, 2019
Copy link
Member

@ydnar ydnar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! Only a few minor changes and this should be good to go.

rr.go Outdated Show resolved Hide resolved
rr.go Outdated Show resolved Hide resolved
cache.go Show resolved Hide resolved
@bookmoons
Copy link
Contributor Author

Thanks a lot for this guys. Will go over all of it.

@ydnar ydnar merged commit bfb9c6f into domainr:master Jun 29, 2019
@ydnar
Copy link
Member

ydnar commented Jun 29, 2019

Thanks!

@bookmoons
Copy link
Contributor Author

Thanks for the review and merge guys.

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.

Add TTL support?
3 participants