refactor: sharky cleanup and simplification of code #3500
Conversation
istae
force-pushed
the
freaky
branch
2 times, most recently
from
1f6576a
to
ec5fe69
Compare
istae
requested review from
a team,
mrekucci and
aloknerurkar
and removed request for
a team
pkg/sharky/store.go
Outdated
| s.wg.Add(1) | ||
| go func() { | ||
| defer sh.slots.wg.Done() | ||
| defer s.wg.Done() | ||
| sh.process() |
There was a problem hiding this comment.
Currently new goroutine is started for each shard instance, would it be possible to avoid these goroutines? Are they really necessary?
All that sh.process() function does is essentially two operations:
slot := sh.slots.Next()
sh.slots.Use(slot)
Would it be possible to just call these two functions in (store.go:136) instead of reading from channel? Probably one more utility function would be needed to get shard, but overall code might be more simpler.
| sl.size = uint32(len(sl.data) * 8) | ||
| sl.head = sl.next(0) | ||
| return err | ||
| sl.data = data |
There was a problem hiding this comment.
Should statement
sl.data = data
be guarded with mutex?
There was a problem hiding this comment.
no, this function is only used during bootup so no mutex is needed
|
This sharky implementation does not have guaranties which shard and location is returned next. As an outcome this could lead to 1) probably more cache invalidation (on hdd/ssd level); and 2) fragmented data. Would it make sense to have sharky to always return first available location in consistent order? This way disk IO will probably hit same sector(s) on disk (probably having cache hit). |
| @@ -44,12 +48,11 @@ type Store struct { | |||
| // - maxDataSize - positive integer representing the maximum blob size to be stored | |||
| func New(basedir fs.FS, shardCnt int, maxDataSize int) (*Store, error) { | |||
There was a problem hiding this comment.
Is store ever writing data down to disk (closing and saving shards)?
There was a problem hiding this comment.
it asks shards to ask slots to save and close
| @@ -28,12 +27,17 @@ var ( | |||
| // - read prioritisation over writing | |||
| // - free slots allow write | |||
| type Store struct { | |||
There was a problem hiding this comment.
This file should be tested.
Order of writes does not guarantee order of reads however. We cannot tell which chunk will be retrieved in the future, so it does not matter which shard we place them. |
istae
force-pushed
the
freaky
branch
2 times, most recently
from
0a9a83e
to
2e4f076
Compare
istae
changed the title
istae
changed the title
|
Consider this again after the major localstore releases. |
Checklist
Description
Write operations now follow a very basic round robin strategy with circular distribution of requests.
Open API Spec Version Changes (if applicable)
Motivation and Context (Optional)
Related Issue (Optional)
Screenshots (if appropriate):
This change is