[utils] Implement ShardedMap #248
Conversation
Pull Request Test Coverage Report for Build 12095
💛 - Coveralls |
| // Lock acquires the lock for a shard that could contain | ||
| // the key. This syntax allows the caller to perform multiple | ||
| // operations while holding the lock for a single shard. | ||
| func (m *ShardedMap) Lock(key string, priority bool) map[string]interface{} { |
There was a problem hiding this comment.
I observe that nothing about this package uses the resulting map[string]interface{} as a map. This could conceivably be a more general ShardedContainer API, where Lock returns an interface{} and it's up to the caller to type-assert it to whatever kind of container they're sharding.
This means you'd also have to parameterize the constructor to construct caller-specified container shards rather than assuming they're maps.
One nice benefit of this approach is that (apart from the need to type-assert the result of Lock) it's more typesafe, in that the caller can say it's a map[string]uint64 instead of having to settle for a map[string]interface{}.
There was a problem hiding this comment.
I observe that nothing about this package uses the resulting map[string]interface{} as a map. This could conceivably be a more general ShardedContainer API, where Lock returns an interface{} and it's up to the caller to type-assert it to whatever kind of container they're sharding.
With the current API, the only thing that we would probably want to be asserted is that interface{} is a pointer (like json.Unmarshal). Otherwise, any modifications to the returned object would be ineffectual (or we return a pointer to what is stored, which may get hairy).
One nice benefit of this approach is that (apart from the need to type-assert the result of Lock) it's more typesafe, in that the caller can say it's a map[string]uint64 instead of having to settle for a map[string]interface{}.
Because the caller (me in MutexMap) doesn't need to parse the value in the map if it doesn't exist, this API allows the caller to avoid unnecessary reflection. However, I see your point about why a ShardedContainer would be cool.
There was a problem hiding this comment.
I think this is a change we could make in a future PR once we have a clear use case in mind (likely just wrapping a ShardedContainer implementation). For now, I'd like to keep this optimized for the use case I have in mind (optimization of the reconciler and storage packages).
patrick-ogrady
requested review from
alex-stone,
cindyxkuang,
qiwu7,
septerr,
yfl92,
bobg and
itstehkman
| @@ -309,7 +309,7 @@ func TestBadgerTrain_Limit(t *testing.T) { | |||
| namespace, | |||
| newDir, | |||
| dictionaryPath, | |||
| 10, | |||
| 50, | |||
There was a problem hiding this comment.
Increased this to stop a flaky test 😢
| }) | ||
|
|
||
| time.Sleep(1 * time.Second) | ||
| assert.NoError(t, g.Wait()) |
There was a problem hiding this comment.
It looks like none of the goroutines in g actually need to return an error, which means you could simplify a bit by using sync.WaitGroup instead.
There was a problem hiding this comment.
I think adding sync.WaitGroup may take more lines/be slightly more complex than just returning nil. I think we would need a wg.Add before spawning each goroutine and would need to call wg.Done at the end of each goroutine. Going to just leave as is for now!
There was a problem hiding this comment.
Appreciate the careful review!
Related: https://github.com/coinbase/rosetta-sdk-go/tree/patrick/priority-lock
This PR adds support for a
ShardedMapthat enables the caller to concurrently write to a "map-like" data structure (where the caller gets a granular lock based onkey).Changes
ShardedMapShardedMapMutexMapto useShardedMapFuture Work
As @bobg mentioned, this
ShardedMapcould be converted to aShardedContainerto allow the caller to store anything they want in a particular shard.