Feat: expose BlockKeyCacheSize and enable WriteThrough datastore options #10614
Conversation
|
We can discuss a bit, because I am not sure when a non-writethrough blockstore/service makes fully sense. Probably on small datastores with very well primed caches that fit all keys and when re-writing the same keys all the time (not sure how common that is to warrant being the current default). I don't think non-WriteThrough makes sense without bloom-filter. And bloom filter is primed on boot, which triggers reading all the keys (horrible in very large datastores).
|
hsanjuan
force-pushed
the
fix/perf-pebble-improvements
branch
from
7f53a7d to
cd96506
Compare
hsanjuan
force-pushed
the
fix/perf-pebble-improvements
branch
from
35cbba2 to
b4b028c
Compare
hsanjuan
changed the title
| var bsopts []bserv.Option | ||
| if cfg.Datastore.WriteThrough { | ||
| bsopts = append(bsopts, bserv.WriteThrough()) | ||
| } | ||
| subAPI.blocks = bserv.New(subAPI.blockstore, subAPI.exchange, bsopts...) |
There was a problem hiding this comment.
This is ok, but is might be nicer to have a blockservice option WithWriteThrough that takes a bool. Then code becomes:
subAPI.blocks = bserv.New(subAPI.blockstore, subAPI.exchange,
bserv.WithWriteThrough(cfg.Datastore.WriteThrough))There was a problem hiding this comment.
Right? We need to change that in boxo. I can take care, but we can merge this and fix that later for next boxo release.
| HashOnRead bool | ||
| BloomFilterSize int | ||
| BlockKeyCacheSize OptionalInteger `json:",omitempty"` | ||
| WriteThrough bool |
There was a problem hiding this comment.
@hsanjuan are you planning to write a migration for existing users (who don't have this in config), or was it intentional to keep existing users stuck with false?
Both feel like a potential headache :)
Perhaps we should switch this from bool to Flag would allow us to control implicit default better, without having to write migration for existing users?
| WriteThrough bool | |
| WriteThrough Flag `json:",omitempty"` |
There was a problem hiding this comment.
If the option is not present, it will take the default which is true? Or you mean the config-parsing code does not know if its false or unset?
| // without performing any reads to check if the incoming blocks are | ||
| // already present in the datastore. Enable for datastores with fast | ||
| // writes and slower reads. | ||
| DefaultWriteThrough = true |
There was a problem hiding this comment.
(if we switch to Flag)
| DefaultWriteThrough = true | |
| DefaultWriteThrough = True |
This enables WriteThrough blockstore and blockservice by default, exposing a new option and BlockKeyCacheSize to control two-queue cache and be able to disable it.