-
Notifications
You must be signed in to change notification settings - Fork 2
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
Add a max TTL for cached entries #12
Add a max TTL for cached entries #12
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the PR!
We usually use the Options pattern to configure services. To see how this looks in practice, have a look at https://github.com/libp2p/go-tcp-transport/blob/c0bf21d689afb213896072e52f225f9126d2ba67/tcp.go#L90-L103.
In this case, we should probably have a func WithMaxTTL(time.Duration) Option
option passed to the constructor (func NewResolver(url string, ...Option) *Resolver
).
By default, we shouldn't limit the TTL (you can use the zero value of r.maxCacheTTL
to mean that we're not applying any limit).
Thanks @marten-seemann. I have updated the PR accordingly. Indeed, the option pattern looks better. It does not require an update of the existing tests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM; we will have to update call sites for the new constructor, so new minor release.
resolver.go
Outdated
@@ -81,6 +102,10 @@ func (r *Resolver) LookupIPAddr(ctx context.Context, domain string) (result []ne | |||
} | |||
} | |||
|
|||
if r.maxCacheTTL != NoMaxCacheTTL { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that we add entries to the cache even if ttl == 0
, can we just maxCacheTTL == 0
to mean that we're not applying any max TTL?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A value of zero means cache is not used by default. If we override 0 meaning to be a special case, users won't be able to define a resolver that revalidates the cache on each query (effectively not using it).
I updated the code to useMaxUint32 as a default. This way, it does not interfere with the possible values for the TTL.
resolver.go
Outdated
func NewResolver(url string) *Resolver { | ||
type Option func(*Resolver) error | ||
|
||
var NoMaxCacheTTL = -1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be a const
, and should not be exported. And with my comment below, we could get rid of it altogether.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This value has now been removed.
resolver.go
Outdated
func NewResolver(url string) *Resolver { | ||
type Option func(*Resolver) error | ||
|
||
func WithMaxCacheTTL(maxCacheTTL uint32) Option { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Docs needed here to indicate the units (IIRC seconds). Adding a shoutout for 0 meaning disabled isn't strictly necessary, but might be nice too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't handle 0 correctly at the moment. We insert every value into the cache. That's something we can fix, but I don't think we have to do it in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, I'm mostly saying the semantics might want to be called out. To some extent it's reasonable to assume users will figure out that a max TTL of 0 means you'll always do a lookup but might be worth specifying.
Waiting until the fix comes in to update the comment is fine too if you're concerned about bugs involving things like Windows thinking the time difference between two events is 0 when it's just small.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A value of 0 is handled properly by cacheTXT
and cacheIPAddr
.
The first line of both method is
if ttl == 0 {
return
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah sorry, I missed that!
If I understand @aschmahmann correctly, he was suggesting was introducing a DisableCache
option, which internally would just set the max TTL to 0. I think that would be a good idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also added a comment on WithMaxCacheTTL
function. It describes the normal behaviour, has well as the behaviour of 0
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Get it. I have added a new option WithCacheDisabled
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't saying that specifically just asking for docs, but that's a reasonable idea/approach @marten-seemann 😁
resolver.go
Outdated
func NewResolver(url string) *Resolver { | ||
type Option func(*Resolver) error | ||
|
||
func WithMaxCacheTTL(maxCacheTTL uint32) Option { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be a time.Duration
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree. This makes the library easier to use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One tiny suggestion, otherwise this LGTM.
resolver_test.go
Outdated
} | ||
|
||
func TestLookupCache(t *testing.T) { | ||
cacheTTL := time.Duration(1) * time.Second |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cacheTTL := time.Duration(1) * time.Second | |
const cacheTTL = time.Second |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated.
I just merged #13. Would you mind rebasing? |
There are also a few failures in the |
@marten-seemann I rebased the code (especially since the interface of the constructor changed). |
Thank you!
@codecov again. This tool has been causing us so many problems.... |
Tests are passing now. |
Fixes #10.