Improve encapsulation of shards #49
Conversation
|
As before: coverage failure was introduced before. |
shard.go
Outdated
| s.lock.RLock() | ||
|
|
||
| var elements = make([]uint32, len(s.hashmap)) | ||
| next := 0 |
There was a problem hiding this comment.
next is local variable and could be defined outside of critical section (before s.lock.RLock())
shard.go
Outdated
| return s.entries.Get(index) | ||
| } | ||
|
|
||
| func (s *cacheShard) copyKeys() ([]uint32, int) { |
There was a problem hiding this comment.
Return values of this function are not obvious (espsecialy second → next) can you add a godoc with information what is the result of this function.
There was a problem hiding this comment.
It looks like it wasn't documented before. How about using named return parameters to defined what's returned?
func (s *cacheShard) copyKeys() (keyes []uint32, length int) {
bigcache.go
Outdated
| } | ||
| } | ||
|
|
||
| func (c *BigCache) getShard(hashedKey uint64) (shard *cacheShard) { | ||
| return c.shards[hashedKey&c.shardMask] | ||
| index := hashedKey & c.shardMask | ||
| return c.shards[index] |
There was a problem hiding this comment.
This could be left untouched.
shard.go
Outdated
| return s.entries.Peek() | ||
| } | ||
|
|
||
| func (s *cacheShard) getIndex(index int) ([]byte, error) { |
There was a problem hiding this comment.
I think this should be renamed. When I see getIndex I think that this function will return an index. How about getEntryUnderIndex or just getEntry?
shard.go
Outdated
| s.lock.Unlock() | ||
| } | ||
|
|
||
| func (s *cacheShard) oldest() ([]byte, error) { |
There was a problem hiding this comment.
How about renaming to getOldestEntry? Below is a method with get... so we should follow one naming scheme and best if functions are named as a verb declaring what they are doing. I'm fine with a convention of naming function with a noun that they produce but then we need to rename all other functions.
|
Thanks, will update tomorrow. |
|
Thanks for quick response. We haven't created any release so far. It's a good idea to start making releases changelog for users. IMO API is stable and we definitely want to keep backward compatibility so we can start with version 1.0. @druminski What do you think? |
|
@janisz Yes, I do agree with you that the API is stable, beside that we don't have plans to do major changes. Therefore we can start release version 1.0. As for writing a changelog, imo it is a good idea. @cristaloleg overall your PR looks good, although please apply @janisz suggestions |
|
@janisz @druminski I'll update PR later today, thank for the review. |
|
@cristaloleg LGTM. I'll create a release and merge it after. |
|
@janisz ok, as you wish. |
|
Done. @cristaloleg Huge thanks for this refactoring. |
|
@janisz no problem, will push next PR in a few days. |
Get,Setmethods are moved frombigcache.gotoshard.goinitNewShardhas additional paramclockcopyCurrentShardMapgetIndexandoldestmethods added to shardgetShardNext PR will introduce
Shardinterface,newShardmethod and additional changes.