Conversation
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
|
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 #3056 +/- ##
==========================================
+ Coverage 58.27% 58.37% +0.10%
==========================================
Files 2076 2083 +7
Lines 171480 171083 -397
==========================================
- Hits 99933 99873 -60
+ Misses 62619 62277 -342
- Partials 8928 8933 +5
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
| IsDelete bool | ||
| } | ||
|
|
||
| // Cache describes a cache kapable of being used by a FlatKV store. |
| lru.totalSize += size | ||
| } | ||
|
|
||
| // Signal that an entry has been interated with, moving it to the to the back of the queue |
There was a problem hiding this comment.
typo: interacted, double to the
|
|
||
| // A utility for assigning keys to shard indices. | ||
| type shardManager struct { | ||
| // A random seed that makmes it hard for an attacker to predict the shard index and to skew the distribution. |
|
|
||
| // Shard returns a shard index in [0, numShards). | ||
| // addr should be the raw address bytes (e.g., 20-byte ETH address). | ||
| func (s *shardManager) Shard(addr []byte) uint64 { |
There was a problem hiding this comment.
suggest adding unit testings for shard_manager.go
| func (s *shardManager) Shard(addr []byte) uint64 { | ||
| h := s.pool.Get().(*maphash.Hash) | ||
| h.SetSeed(s.seed) | ||
| h.Reset() |
There was a problem hiding this comment.
curious how does the h.SetSeed(s.seed) then h.Resest() next line work work?
There was a problem hiding this comment.
h.Reset() doesn't remove the seed, it just removes the accumulated digest. But digging deeper into the implementation, it looks like h.SetSeed() also removes any accumulated digest, so this call was redundant. Removed.
|
|
||
| // Creates a new Sharder. Number of shards must be a power of two and greater than 0. | ||
| func NewShardManager(numShards uint64) (*shardManager, error) { | ||
| if numShards <= 0 || (numShards&(numShards-1)) != 0 { |
There was a problem hiding this comment.
nit, numShards == 0 for uint64
| } | ||
|
|
||
| // Create a new LRU queue. | ||
| func NewLRUQueue() *lruQueue { |
There was a problem hiding this comment.
Exported constructor returns unexported type lru_queue.go, shard_manager.go
NewLRUQueue() and NewShardManager() are exported but return unexported types. Since both are internal, how about unexport the constructors: newLRUQueue, newShardManager.
There was a problem hiding this comment.
constructors are no longer exported
| key []byte, | ||
| // the size of the key + value | ||
| size int, | ||
| ) { |
There was a problem hiding this comment.
nit: Push() accepts negative values
There was a problem hiding this comment.
changed type to uint64
| if err != nil { | ||
| keys[k] = types.BatchGetResult{Error: err} | ||
| } else { | ||
| keys[k] = types.BatchGetResult{Value: val, Found: found} |
There was a problem hiding this comment.
looks the error result is silently swallow here
There was a problem hiding this comment.
Made it so that if any of the get calls fail, then this entire method returns an error. Should simplify caller logic (they won't have to scan the entire map before using the results).
| // The key to update. | ||
| Key []byte | ||
| // The value to set. If nil, the key will be deleted. | ||
| Value []byte |
There was a problem hiding this comment.
in the comment, when the value is nil, the key will be deleted. with this, do we still ned IsDelete ?
There was a problem hiding this comment.
Good point, simplified
|
Is superseded by #3072. All fixes for comments in this PR have been carried over into that new branch. |
Describe your changes and provide context
These are helper files for the FlatKV cache. Not general purpose utilities, but also not the core cache implementation.
Testing performed to validate your change
Tested in main branch (unit tests, benchmark, etc)