[#421] Try using badger for the write-cache #462
No reviewers
Labels
No labels
P0
P1
P2
P3
badger
frostfs-adm
frostfs-cli
frostfs-ir
frostfs-lens
frostfs-node
good first issue
triage
Infrastructure
blocked
bug
config
discussion
documentation
duplicate
enhancement
go
help wanted
internal
invalid
kludge
observability
perfomance
question
refactoring
wontfix
No milestone
No project
No assignees
5 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference: TrueCloudLab/frostfs-node#462
Loading…
Add table
Reference in a new issue
No description provided.
Delete branch "ale64bit/frostfs-node:feature/421-write-cache-badger"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Signed-off-by: Alejandro Lopez a.lopez@yadro.com
[#421] Try using badger for the write-cacheto WIP: [#421] Try using badger for the write-cache1f170cad91
to3d7c992409
WIP: [#421] Try using badger for the write-cacheto [#421] Try using badger for the write-cachehttps://dgraph.io/docs/badger/get-started/#garbage-collection
I think we should add some GC goroutine.
3d7c992409
to0df45009ed
@ -0,0 +13,4 @@
case <-c.closeCh:
return
case <-t.C:
for {
Unfortunately linter doesn't allow writing:
because of empty body, even though the function call is stateful.
hack the linter: add logging:)
done :)
Done
0df45009ed
toeb5c961501
@ -0,0 +39,4 @@
func (c *cache) runFlushLoop() {
ch := c.closeCh
c.wg.Add(1)
go func() {
Looks redundant. Why we need to read from
closeCh
in routine and do nothing?fixed
eb5c961501
to590192ce4a
Besides the benchmarks in this PR, I added some write test results in the original issue: #421 (comment)
590192ce4a
to4e445deb59
Added a config to select the writecache implementation.
@ -126,6 +128,7 @@ type shardCfg struct {
writecacheCfg struct {
enabled bool
typ writecacheconfig.Type
I hope this option is just for testing?
wdym?
If we are going to support two DB's, we will have to provide an update procedure. If, for example, I changed bbolt to badger in the config, then the system must first flush objects from bbolt, and then change the storage to badger.
I see. Yes, I agree. This option is mostly intended to be able to use both during a transition period. But maybe @fyrchik knows better.
4e445deb59
toe7358b9808
e7358b9808
toc95d71346e
c95d71346e
to3b53f8fbb1
@fyrchik what's the plan with this PR?
@ -0,0 +118,4 @@
var err error
if c.db != nil {
err = c.db.Close()
if err != nil {
But if it is successfully closed, it is still non-nil and
db
can keep semi-closed state. Is it correct? Do not we need to assignc.db = nil
always?Hmm, that's interesting. I just copied it from the existing writecache:
https://git.frostfs.info/TrueCloudLab/frostfs-node/src/branch/master/pkg/local_object_storage/writecache/writecache.go#L166-L173
I think you are right. But let's address it in a different PR, because I'm afraid @fyrchik will say "it's unrelated to the PR" or smth.
3b53f8fbb1
to6ac9730106
6ac9730106
to453ff301d1
@ -719,2 +714,2 @@
writecache.WithLogger(c.log),
)
switch wcRead.typ {
case writecacheconfig.TypeBBolt:
What do you think about hiding the implementation behind an interface?
Basically, we provide type and type specific options, the implementation picks option for type.
I rather don't do that if we are not planning to maintain multiple implementations.
@ -38,0 +51,4 @@
return writecacheconfig.TypeBadger
}
panic(fmt.Sprintf("invalid writecache type: %q", t))
empty string should be equal to
bbolt
for compatibilitydone
@ -39,3 +41,3 @@
}
func newCustomShard(t testing.TB, rootPath string, enableWriteCache bool, wcOpts []writecache.Option, bsOpts []blobstor.Option, metaOptions []meta.Option) *shard.Shard {
func newCustomShard(t testing.TB, rootPath string, enableWriteCache bool, wcOpts writecacheconfig.Options, bsOpts []blobstor.Option, metaOptions []meta.Option) *shard.Shard {
Why not provide a pointer? (so we just use
nil
in the caller as before.I prefer the explicit type at the caller site.
Ok, but this makes me think in situations where I don't care (
enabledWriteCache=false
)so, do you want me change it or ... ?
As you wish, not worth debating, just providing an argument.
@ -0,0 +80,4 @@
continue
}
// We put objects in batches of fixed size to not interfere with main put cycle a lot.
This comment may no longer be valid. The problem with
bolt
is that in-progressView
tx can prevent remapping. For badger this could be optimized.done
Ok, we still have the old algorithm. Could you create a task to improve this then? (I believe we can send inside the transaction without intermediate slice).
#568
@ -0,0 +111,4 @@
check(t, mb, bs, objects)
})
t.Run("flush on moving to degraded mode", func(t *testing.T) {
These tests check logical behaviour which should hold true for any writecache implementation, what about making them generic (sth similar to
blobstor/internal/blobstortest
)?done
@ -0,0 +198,4 @@
return obj, data
}
func initWC(t *testing.T, wc writecache.Cache) {
This is removed in master, can we remove it here too?
Initially it was a separate function because it had auxiliary logic, not it has not.
done
@ -0,0 +14,4 @@
go func() {
defer c.wg.Done()
t := time.NewTicker(defaultGCInterval)
Why not make this a tunable parameter?
done
@ -0,0 +22,4 @@
case <-c.closeCh:
return
case <-t.C:
for c.db.RunValueLogGC(0.5) == nil {
Magic constant, can we move it to
default...
too?it's not magic, added a comment.
Everything is magic until one finally understands :)
@ -0,0 +18,4 @@
// Returns ErrNotInitialized if write-cache has not been initialized yet.
// Returns ErrOutOfSpace if saving an object leads to WC's size overflow.
// Returns ErrBigObject if an objects exceeds maximum object size.
func (c *cache) Put(ctx context.Context, prm common.PutPrm) (common.PutRes, error) {
So we no longer store object in the fstree here?
that's right.
@ -0,0 +10,4 @@
func (c *cache) estimateCacheSize() uint64 {
onDiskSize, _ := c.db.EstimateSize(nil)
c.metrics.SetEstimateSize(onDiskSize, 0)
So we update this before each put?
What about doing this after?
Also, do we need to do this after each GC cycle?
I kept the same behavior from the existing writecache. We can change it in a separate PR for both.
I'm not sure why to do it after GC, since it might not remove anything from disk until the compactor runs anyway, I believe.
@ -0,0 +41,4 @@
opts.PrefetchValues = false
it := tx.NewIterator(opts)
defer it.Close()
for it.Rewind(); it.Valid(); it.Next() {
Wow, this may affect startup time significantly. As I understand, badger doesn't have a key with a number of items?
Not that I know of.
https://discuss.dgraph.io/t/count-of-items-in-db/7549
Ok, I have created #561
@ -0,0 +6,4 @@
// OpenDB opens a badger instance for write-cache. Opens in read-only mode if ro is true.
func OpenDB(p string, ro bool) (*badger.DB, error) {
return badger.Open(badger.DefaultOptions(p).WithReadOnly(ro).WithLoggingLevel(badger.ERROR))
What kind of logger does it use? Can we use
zap
there? (we have many possible customizations, and currently we can make an assumption thatzap
is the only thing we need to configure).done
453ff301d1
to4d88bdd4e5