Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a LevelDB-backed implementation of the httpcache.Cache interface, providing a persistent, disk-based caching option alongside the existing in-memory and Ristretto cache implementations.
Changes:
- Implements a new LevelDB cache backend with Get, Put, Del, and Close methods
- Adds comprehensive tests and benchmarks for the LevelDB cache
- Adds proper cleanup (defer Close()) to existing Ristretto tests
- Updates dependencies to include goleveldb and its transitive dependencies
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| leveldb/leveldb.go | Core implementation of LevelDB cache with error logging and io.Closer interface |
| leveldb/leveldb_test.go | Basic functional and concurrency tests for LevelDB cache |
| leveldb/leveldb_bench_test.go | Performance benchmarks for Get, Put, and mixed operations |
| ristretto/ristretto_test.go | Added defer cache.Close() for proper resource cleanup in tests |
| go.mod | Added goleveldb dependency |
| go.sum | Updated with goleveldb and its transitive dependencies |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| // Make returns a cache using the specified db instance as the underlying storage. | ||
| func Make(db *leveldb.DB) *Cache { |
There was a problem hiding this comment.
The Make function should validate that the db parameter is not nil to prevent panics when methods are called on the Cache. Add a nil check and return an error or panic with a descriptive message if db is nil.
| func Make(db *leveldb.DB) *Cache { | |
| func Make(db *leveldb.DB) *Cache { | |
| if db == nil { | |
| panic("leveldb: nil *leveldb.DB passed to leveldb.Make") | |
| } |
| @@ -0,0 +1,61 @@ | |||
| package leveldb | |||
There was a problem hiding this comment.
Add a package-level documentation comment to describe the leveldb package, similar to the pattern used in ristretto/ristretto.go. This should explain what the package provides, its use case, and include a simple example. Package documentation improves discoverability and usability.
| func benchmarkGet(size int) func(b *testing.B) { | ||
| return func(b *testing.B) { | ||
| path := filepath.Join(b.TempDir(), "cache.db") | ||
| cache, err := leveldb.New(path) | ||
| require.NoError(b, err) | ||
| defer cache.Close() |
There was a problem hiding this comment.
The cache instance created in this benchmark is not properly closed. This can lead to resource leaks, especially in benchmarks that run multiple iterations. Add a defer statement after line 16 to close the cache.
| func benchmarkPut(size int) func(b *testing.B) { | ||
| return func(b *testing.B) { | ||
| path := filepath.Join(b.TempDir(), "cache.db") | ||
| cache, err := leveldb.New(path) | ||
| require.NoError(b, err) | ||
| defer cache.Close() |
There was a problem hiding this comment.
The cache instance created in this benchmark is not properly closed. This can lead to resource leaks, especially in benchmarks that run multiple iterations. Add a defer statement after line 44 to close the cache.
| func BenchmarkLevelDBCacheMixed(b *testing.B) { | ||
| path := filepath.Join(b.TempDir(), "cache.db") | ||
| cache, err := leveldb.New(path) | ||
| require.NoError(b, err) | ||
| defer cache.Close() |
There was a problem hiding this comment.
The cache instance created in this benchmark is not properly closed. This can lead to resource leaks, especially in benchmarks that run multiple iterations. Add a defer statement after line 66 to close the cache.
| func BenchmarkLevelDBCacheParallelMixed(b *testing.B) { | ||
| path := filepath.Join(b.TempDir(), "cache.db") | ||
| cache, err := leveldb.New(path) | ||
| require.NoError(b, err) | ||
| defer cache.Close() |
There was a problem hiding this comment.
The cache instance created in this benchmark is not properly closed. This can lead to resource leaks, especially in benchmarks that run multiple iterations. Add a defer statement after line 89 to close the cache.
| github.com/cespare/xxhash/v2 v2.3.0 // indirect | ||
| github.com/davecgh/go-spew v1.1.1 // indirect | ||
| github.com/dustin/go-humanize v1.0.1 // indirect | ||
| github.com/golang/snappy v0.0.0-20180518054509-2e65f85255db // indirect |
There was a problem hiding this comment.
This version of golang/snappy is from 2018 (commit 2e65f85255db). Consider updating to a more recent version for potential bug fixes and performance improvements. The goleveldb dependency may be pulling in an outdated transitive dependency.
| github.com/golang/snappy v0.0.0-20180518054509-2e65f85255db // indirect | |
| github.com/golang/snappy v0.0.4 // indirect |
| // Cache is an implementation of httpcache.Cache with leveldb storage | ||
| type Cache struct { | ||
| db *leveldb.DB | ||
| } |
There was a problem hiding this comment.
Add compile-time interface checks to ensure Cache implements httpcache.Cache and io.Closer, similar to the pattern used in ristretto/ristretto.go. Add these lines after the Cache struct definition: var _ httpcache.Cache = (*Cache)(nil) and var _ io.Closer = (*Cache)(nil). This ensures the interface is properly implemented and catches any breaking changes at compile time.
1 participant
Scope of changes
Adds LevelDB stateful cache.
Fixes SC-36558
Type of change
Author checklist