Skip to content

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

Merged
merged 5 commits into from
Apr 20, 2024
Merged

Support different cache implementations #4

merged 5 commits into from
Apr 20, 2024

Conversation

thrawn01
Copy link
Contributor

@thrawn01 thrawn01 commented Apr 13, 2024

Purpose

To support alternative hot and main cache implementations that may have better performance under high concurrency workloads.

Implementation

  • Introduced the Cache interface
  • Moved the Modifications from original library section to the bottom of the readme.
  • Renamed cache to mutexCache and renamed some of it's internal variables
  • Modified mutexCache to implement the new Cache interface
  • Moved eviction for mutexCache from populateCache() into mutexCache.Add()
  • Added Options.CacheFactory to allow the initialization of third party cache implementations
  • Added an optional cache implementation for github.com/maypok86/otter
  • Added CacheStats.Rejected to count the number of items rejected by the cache.
  • Add documentation for using otter in groupcache.
  • Renamed SpawnServer() and SpawnDaemon()toListenAndServe()`

Usage

import "github.com/groupcache/groupcache-go/v3/contrib"

// Create a new groupcache instance with a custom cache implementation
instance := groupcache.New(groupcache.Options{
    CacheFactory: func(maxBytes int64) (groupcache.Cache, error) {
        return contrib.NewOtterCache(maxBytes)
    },
    HashFn:    fnv1.HashBytes64,
    Logger:    slog.Default(),
    Transport: t,
    Replicas:  50,
})

See #1

@thrawn01 thrawn01 self-assigned this Apr 13, 2024
@thrawn01 thrawn01 marked this pull request as draft April 13, 2024 20:20
@thrawn01 thrawn01 marked this pull request as ready for review April 14, 2024 23:42
@thrawn01
Copy link
Contributor Author

This is ready for review. @udhos, @gedw99 @Tochemey @Jvb182 @Baliedge @MatthewEdge

I'm also pinging @maypok86 to thank him for the wonderful otter cache! (I hope I'm using it correctly 😄 )

Please review and let me know what I missed or broke.

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

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?

Copy link
Contributor Author

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.

Copy link
Contributor

@Tochemey Tochemey left a 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.

@Tochemey
Copy link
Contributor

@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.

@maypok86
Copy link

@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.

  1. The ristretto approach. We simply discard the inserted item if a high contention is detected. This is a very rare approach and very controversial. To be honest, I really don't like it.
  2. Rejection of an item that is too large. This is quite common, because the insertion of such an item will lead to a very strong drop in the hit rate. This is used in otter.

@thrawn01
Copy link
Contributor Author

@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.

Yes it's pretty common.

I wrote a section in the README called Cache Size Implications to make users aware of the possibility of rejections. TLDR; Since we are using Otter for both main and hot caches and the hot cache is 1/8th the size of the main cache, and otter's max cost is calculated as 1/10th the max size of the cache it's possible that large items will be rejected from the hot cache.

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.

@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.

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 contrib isn't used by your project. It also becomes a great example of how to add your own third-party cache!

@thrawn01 thrawn01 merged commit 3b42ce3 into main Apr 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants