-
Notifications
You must be signed in to change notification settings - Fork 5
Support different cache implementations #4
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
Conversation
// ShutdownServer shuts down the server started when calling SpawnServer() | ||
ShutdownServer(ctx context.Context) error | ||
// Shutdown shuts down the server started when calling ListenAndServe() | ||
Shutdown(ctx context.Context) error |
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.
@thrawn01 do we have some shutdown timeout(graceful shutdown) concept here or it is not necessary?
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.
Are you talking about the context.Context
?
Yes graceful shutdown's can sometimes take a while. (if there is a handler that is stuck for instance) In such a case it's always best to provide a way to timeout a shutdown. I've been bit by this before in production where a service got stuck in shutdown and we had to forceably shutdown the service losing all of the data in the internal buffers that should have been sync'd to disk. We started adding context.Context
to all our shutdown methods after that. It's one of those things that you don't think is an issue, until it is.
Golang's standard http server Shutdown()
method takes a context.Context
for this same reason.
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.
@thrawn01 I believe like the discovery the contrib cache should go into another repository because of the different cache integration developers may want to add.
@thrawn01 Also for the cache rejected stats, is it a standard practice out there to reject item? If not I think then we are leaking otter custom feature here. |
You can, it's your decision. I will even be a little glad, since I was not ready for the popularity of the otter. But I would like to tell you a little about the rejection. There are several ways in which this can happen in different caches.
|
Yes it's pretty common. I wrote a section in the README called I considered making Otter the default cache implementation. But your reaction confirms my concern that users might be caught off guard, not understand why the hot cache might have a higher miss ratio than they expect.
It's such a small include, it doesn't need it's own repo. Also, thanks to go mods Module graph pruning. Otter should not become a dependency of your project if |
Purpose
To support alternative hot and main cache implementations that may have better performance under high concurrency workloads.
Implementation
Cache
interfaceModifications from original library
section to the bottom of the readme.cache
tomutexCache
and renamed some of it's internal variablesmutexCache
to implement the newCache
interfacemutexCache
frompopulateCache()
intomutexCache.Add()
Options.CacheFactory
to allow the initialization of third party cache implementationsgithub.com/maypok86/otter
CacheStats.Rejected
to count the number of items rejected by the cache.otter
in groupcache.SpawnServer() and
SpawnDaemon()to
ListenAndServe()`Usage
See #1