-
Notifications
You must be signed in to change notification settings - Fork 666
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 CacheLatestTtlSecs to allow expiration of latest schemas #1106
Conversation
|
1 similar comment
|
// Clear clears the cache | ||
func (c *LRUCache) Clear() { | ||
c.cacheLock.Lock() | ||
for key, value := range c.lruElements { |
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.
Is there a more efficient way to clear than iterating through all the elements?
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 believe this is idomatic go before go 1.21, where they added a clear()
method. In any case, these maps are not expected to get that large as they store one entry per subject
} else { | ||
schemaToIDCache = cache.NewMapCache() | ||
idToSchemaCache = cache.NewMapCache() | ||
schemaToVersionCache = cache.NewMapCache() | ||
versionToSchemaCache = cache.NewMapCache() | ||
latestToSchemaCache = cache.NewMapCache() | ||
} | ||
if conf.CacheLatestTTLSecs > 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.
I'm not familiar with the use case here, but we can have a LruCache with TTL on individual entries, why do we want to clear the entire cache vs having TTLs on each cache entry
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.
The TTL is just to ensure that the latest entries are refreshed every so often. It's not necessary to have a TTL per entry.
} | ||
handle := &client{ | ||
restService: restService, | ||
schemaToIDCache: schemaToIDCache, | ||
idToSchemaCache: idToSchemaCache, | ||
schemaToVersionCache: schemaToVersionCache, | ||
versionToSchemaCache: versionToSchemaCache, | ||
latestToSchemaCache: latestToSchemaCache, | ||
} | ||
return handle, nil |
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.
Coulse use runtime.SetFinalizer
with handle
that stops the ticker and a done channel to exit the for loop in the spawned goroutine
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.
Yes, let me make that change
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.
@emasab , I added the finalizer
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. Thanks @rayokota !
Add CacheLatestTtlSecs to allow expiration of latest schemas