Helper files for the flatKV cache implementation#3072
Helper files for the flatKV cache implementation#3072cody-littley wants to merge 2 commits intomainfrom
Conversation
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3072 +/- ##
==========================================
+ Coverage 58.35% 58.36% +0.01%
==========================================
Files 2080 2087 +7
Lines 171659 171796 +137
==========================================
+ Hits 100170 100268 +98
- Misses 62564 62603 +39
Partials 8925 8925
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
| } | ||
|
|
||
| func (c *cachedKeyValueDB) Get(key []byte) ([]byte, error) { | ||
| val, found, err := c.cache.Get(key, true) |
There was a problem hiding this comment.
Why we don't need to fallback to c.db.get() on cache miss? Usually what we do is:
- read from cache
- on cache miss -> read from db -> backfill cache
| func (cb *cachedBatch) Set(key, value []byte) error { | ||
| cb.pending = append(cb.pending, CacheUpdate{Key: key, Value: value}) | ||
| return cb.inner.Set(key, value) | ||
| } | ||
|
|
||
| func (cb *cachedBatch) Delete(key []byte) error { |
There was a problem hiding this comment.
Set and Delete append CacheUpdate{Key: key, Value: value} directly using the caller's slices. If the caller reuses or mutates those byte slices after calling Set/Delete but before Commit, the pending updates will contain corrupted data?
| return err | ||
| } | ||
| if err := cb.cache.BatchSet(cb.pending); err != nil { | ||
| return fmt.Errorf("failed to update cache after commit: %w", err) |
There was a problem hiding this comment.
if inner.Commit() succeeds but cache.BatchSet() fails, the method returns an error. Would that cause any confusion to the caller since the data is already persisted? Is cache update best-effort? If so maybe we just return nil but log the error?
| } | ||
|
|
||
| // Combine a cache and a key-value database to create a new key-value database with caching. | ||
| func NewCachedKeyValueDB(db types.KeyValueDB, cache Cache) types.KeyValueDB { |
There was a problem hiding this comment.
the constructor returns KeyValueDB and func BatchGet is not defined in the interface
| return c.readFunc(key) | ||
| } | ||
|
|
||
| func (c *noOpCache) BatchGet(keys map[string]types.BatchGetResult) error { |
There was a problem hiding this comment.
what's the diff between not-found vs empty-value in BatchGet
| lru.totalSize += size | ||
| } | ||
|
|
||
| // Signal that an entry has been interated with, moving it to the back of the queue |
| ) { | ||
| if elem, ok := lru.entries[string(key)]; ok { | ||
| entry := elem.Value.(*lruQueueEntry) | ||
| lru.totalSize += size - entry.size |
There was a problem hiding this comment.
any chance this leads to underflow?
| cache Cache | ||
| } | ||
|
|
||
| // Combine a cache and a key-value database to create a new key-value database with caching. |
3 participants
Describe your changes and provide context
Replaces PR #3056
Helper files for the FlatKV cache implementation.
Testing performed to validate your change
Local testing, long running benchmark.