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

Add CacheLatestTtlSecs to allow expiration of latest schemas #1106

Merged
merged 19 commits into from
Nov 20, 2023

Conversation

rayokota
Copy link
Member

@rayokota rayokota commented Nov 15, 2023

Add CacheLatestTtlSecs to allow expiration of latest schemas

Copy link

cla-assistant bot commented Nov 15, 2023

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ rayokota
❌ ConfluentJenkins
You have signed the CLA already but the status is still pending? Let us recheck it.

1 similar comment
Copy link

cla-assistant bot commented Nov 15, 2023

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ rayokota
❌ ConfluentJenkins
You have signed the CLA already but the status is still pending? Let us recheck it.

// Clear clears the cache
func (c *LRUCache) Clear() {
c.cacheLock.Lock()
for key, value := range c.lruElements {

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?

Copy link
Member Author

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 {

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

Copy link
Member Author

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
Copy link
Contributor

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

Copy link
Member Author

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

Copy link
Member Author

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

Copy link
Contributor

@emasab emasab left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @rayokota !

@rayokota rayokota merged commit af4a5f8 into master Nov 20, 2023
2 of 3 checks passed
@rayokota rayokota deleted the add-ttl-latest branch November 20, 2023 19:02
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.

None yet

4 participants