Implemented new Append() method #203
Conversation
shard.go
Outdated
| @@ -25,40 +25,52 @@ type cacheShard struct { | |||
| stats Stats | |||
| } | |||
|
|
|||
| func (s *cacheShard) get(key string, hashedKey uint64) ([]byte, error) { | |||
| s.lock.RLock() | |||
| func (s *cacheShard) get(key string, hashedKey uint64, skipLock bool) ([]byte, error) { | |||
There was a problem hiding this comment.
I'm not a fan of bool param here. It's easy to misuse such an API.
WDYT about left get as it was and create a separate method getWithoutLock (or a better name if you've) ?
There was a problem hiding this comment.
Ah, and the same regarding set method 😉
There was a problem hiding this comment.
I am really missing parameter default values here (which would be false of cause to not change the API).
But I have no better idea. I could clone the method but this will result in a "lot" of duplicate code. I would basically implement the method again just leaving out the lock lines?
Since those calls are not public I think we can live with it. What do you think?
There was a problem hiding this comment.
There is no need for a default params. Both of the functions are from different worls - one is lock-free and another is not.
Combining them is a way to make an error quite easily.
That's why I suggest to split them into 2 different. Little code duplication isn't a problem (especially here).
|
btw, looks like your branch is outdated and you've a conflict, can you fix it too? pretty please :) |
|
Fixed it, kinda "redid" the set()/get() part because I didnot want to screw up (merge conflict). Please check again carefully, thanks! |
bigcache.go
Outdated
| @@ -134,6 +134,13 @@ func (c *BigCache) Set(key string, entry []byte) error { | |||
| return shard.set(key, hashedKey, entry) | |||
| } | |||
|
|
|||
| // Append appends entry under existing key | |||
There was a problem hiding this comment.
Can you elaborate this? What if key does not exist and how it differs from Set
bigcache_test.go
Outdated
|
|
||
| // when | ||
| cache.Append("key", value) | ||
| cache.Append("key", value) |
There was a problem hiding this comment.
We need a test to append different value and to run multiple appends in parallel.
There was a problem hiding this comment.
improved the test, the test is already marked with t.Parallel() is that what you mean?
There was a problem hiding this comment.
I thought about doing sometehing like we did here https://github.com/allegro/bigcache/blob/master/bigcache_test.go#L405-L406 just to check if locking is done properly.
There was a problem hiding this comment.
ok good call, I implemented something like you mentioned and found another bug :)
One problem remains: the race detector triggers but I have no idea why. Do you guys have experience with it? Is it "valid" all the time?
There was a problem hiding this comment.
Ok fixed it, it was because I was using the "err" variable from the outer scope! :)
bigcache_test.go
Outdated
| // given | ||
| cache, _ := NewBigCache(DefaultConfig(5 * time.Second)) | ||
| key := "key" | ||
| value1 := make([]byte, 50) |
There was a problem hiding this comment.
| value1 := make([]byte, 50) | |
| value1 := blob('a', 50) |
|
+1, need this for my project |
|
@user3415653 can you provide more details? Why do you need this feature? (it's ready to merge, but I'm curious about a bit wider context, probably we can improve bigcache in the future). Thanks in advance. |
|
@janisz PTAL 😉 |
|
Thanks @snacker81 |
|
@cristaloleg I think it's too late now, but this should've been a v2.2.0 bump instead of v2.1.7 as it adds a new API. |
|
@mxplusb good point! will publish v2.2.0 |
@cristaloleg No problem thanks for the solution-oriented process :) |
Implemented new Append() method with proper locking. Without this method you would need to wrap a Get()/Set() part with your own locks hurting performance.
Fixes #158