Remove-Get concurrency fix #27
Conversation
iulianpascalau
commented
Sep 18, 2023
•
iulianpascalau
commented
- added unit test for the edge-case
- fixed the persister edge-case
- added & integrated a new type of cache that is removal-aware
leveldb/leveldbSerial.go
Outdated
| @@ -262,6 +297,10 @@ func (s *SerialDB) putBatch() error { | |||
| result := <-ch | |||
| close(ch) | |||
|
|
|||
| s.mutBatch.Lock() | |||
There was a problem hiding this comment.
not a great solution, this can just set to nil the writing batch if another go-routine called putBatch and managed to set the writingBatch
There was a problem hiding this comment.
right, reverted
- added & integrated removalTrackingCache implementation
iulianpascalau
changed the title
storageUnit/storageunit_test.go
Outdated
| return nil | ||
| }, | ||
| GetCalled: func(key []byte) ([]byte, error) { | ||
| assert.Fail(t, "should have not called Has") |
There was a problem hiding this comment.
| assert.Fail(t, "should have not called Has") | |
| assert.Fail(t, "should have not called Get") |
storageUnit/storageunit_test.go
Outdated
| return nil | ||
| }, | ||
| GetCalled: func(key []byte) ([]byte, error) { | ||
| assert.Fail(t, "should have not called Has") |
There was a problem hiding this comment.
| assert.Fail(t, "should have not called Has") | |
| assert.Fail(t, "should have not called Get") |
go.mod
Outdated
| @@ -5,7 +5,7 @@ go 1.20 | |||
| require ( | |||
| github.com/hashicorp/golang-lru v0.6.0 | |||
| github.com/multiversx/concurrent-map v0.1.4 | |||
| github.com/multiversx/mx-chain-core-go v1.2.16 | |||
| github.com/multiversx/mx-chain-core-go v1.2.17-0.20230919104727-566f55d213ab | |||
There was a problem hiding this comment.
don't forget proper tag
There was a problem hiding this comment.
after testing will do 👍
| if found { | ||
| return true | ||
| } | ||
| _, found = b.cachedData[string(key)] |
There was a problem hiding this comment.
Could be a situation when both, cachedData[key} and previouslyRemoved[key], return true?
There was a problem hiding this comment.
Yes. The situation is the following:
the key existed and a call to Delete was made. The batch was written, and the data was deleted from the persister
and the new batch was created, the previously removed data was passed to the new batch. Then, immediately we call Put with the same key. Now, both caches contain the same key.
All is fine because in this case, we want the IsRemoved function to return false. Added a test with this exact scenario.
rtcache/removalTrackingCache.go
Outdated
|
|
||
| // Clear is used to completely clear both caches. | ||
| func (cache *removalTrackingCache) Clear() { | ||
| cache.mutCriticalArea.RLock() |
There was a problem hiding this comment.
right, to preserve the consistency as much as we can
| defer mock.mut.Unlock() | ||
|
|
||
| _, found := mock.data[string(key)] | ||
|
|
There was a problem hiding this comment.
Missing here: if found { return found, !found} ?
There was a problem hiding this comment.
right, changed also the other return as it was badly written
testscommon/cacherMock.go
Outdated
|
|
||
| mock.data[string(key)] = value | ||
|
|
||
| return !found, found |
There was a problem hiding this comment.
return found, !found ?
There was a problem hiding this comment.
right, thanks
types/interface.go
Outdated
| UnknownRemovalStatus RemovalStatus = "unknown" | ||
| // ExplicitlyRemovedStatus defines the explicitly removed status | ||
| ExplicitlyRemovedStatus RemovalStatus = "explicitly removed" | ||
| // NotRemovedStatus the key is not removed |
There was a problem hiding this comment.
// NotRemovedStatus defines the not removed status ?
iulianpascalau
dismissed stale reviews from SebastianMarian and sstanculeanu
via
2294bdc
- added compiling protection to all cachers
