Move the cache implementation behind an interface #614
Conversation
github-actions
bot
added
area/CLI
pkg/cmd/server/cacheconfig.go
Outdated
| @@ -24,7 +25,7 @@ const ( | |||
| ) | |||
|
|
|||
| // Complete converts the cache config into a ristretto cache config. | |||
| "github.com/dgraph-io/ristretto" | ||
| ) | ||
|
|
||
| // NewCache creates a new ristretto cache from the given config. |
There was a problem hiding this comment.
Should we stick to the "return structs accept interfaces" mantra here?
There was a problem hiding this comment.
We can, but it does mean that NewCache's signature will change based on the platform, which may cause issues in certain compilation targets
There was a problem hiding this comment.
I don't get it. In this case it's always going to be a ristretto cache coming out of cache_ristretto. The method should probably be called NewRistrettoCache, and it will be totally missing on the wasm arch. If there were a wasm compatible version it would be called NewHashmapCache or something, and its version of wrapped would be specific to that cache impl.
There was a problem hiding this comment.
If there were a wasm compatible version it would be called NewHashmapCache or something
Actually, no. The whole point is that it is NewCache regardless of platform, so that when it is invoked outside of the package, the caller does not need to know on which platform the code is operating. Otherwise, it would fail to compile.
josephschorr
force-pushed
the
cache-interface
branch
from
3ea851a to
f45b802
Compare
|
Updated |
| "github.com/dgraph-io/ristretto" | ||
| ) | ||
|
|
||
| // NewCache creates a new ristretto cache from the given config. |
There was a problem hiding this comment.
I don't get it. In this case it's always going to be a ristretto cache coming out of cache_ristretto. The method should probably be called NewRistrettoCache, and it will be totally missing on the wasm arch. If there were a wasm compatible version it would be called NewHashmapCache or something, and its version of wrapped would be specific to that cache impl.
…rent implementations based on platform
To allow for different implementations based on platform