writecache: Improve flushing scheme for badger #641

Merged
fyrchik merged 1 commit from acid-ant/frostfs-node:bugfix/568-imprv-badger-flush into master 2023-08-30 17:22:32 +00:00
Member

Close #568

Signed-off-by: Anton Nikiforov an.nikiforov@yadro.com

Close #568 Signed-off-by: Anton Nikiforov <an.nikiforov@yadro.com>
acid-ant requested review from storage-core-committers 2023-08-23 11:54:01 +00:00
acid-ant requested review from storage-core-developers 2023-08-23 11:54:04 +00:00
acid-ant force-pushed bugfix/568-imprv-badger-flush from 47cfd47377 to bcb90c455b 2023-08-23 11:54:43 +00:00 Compare
fyrchik reviewed 2023-08-23 16:25:36 +00:00
@ -23,1 +23,4 @@
flushCh chan *objectSDK.Object
// scheduled4Flush contains objects scheduled for flush via flushCh
// helps to avoid multiple flushing of one object
scheduled4Flush map[oid.Address]any
Owner

If it is a set, it is better to use struct{}, not any, as struct{} has zero size and thus could be optimized by the compiler.

If it is a set, it is better to use `struct{}`, not `any`, as `struct{}` has zero size and thus could be optimized by the compiler.
Author
Member

Thanks, updated.

Thanks, updated.
fyrchik marked this conversation as resolved
@ -38,0 +58,4 @@
if kv.StreamDone {
return nil
}
if c.scheduled >= flushBatchSize {
Owner

Flush batch size was there in bbolt, because long View transactions prevented database from growing in size.
In badger we may not need this.

Flush batch size was there in `bbolt`, because long `View` transactions prevented database from growing in size. In badger we may not need this.
Author
Member

The reason I decided to continue to use it is to allow changing mode for writecache. If wc will be under pressure and for any reason we need to change mode, this will be possible once all objects were scheduled. It may take some time, external call may fail by timeout.
Is It acceptable for us? Or it is better to interrupt some time?

The reason I decided to continue to use it is to allow changing mode for writecache. If wc will be under pressure and for any reason we need to change mode, this will be possible once all objects were scheduled. It may take some time, external call may fail by timeout. Is It acceptable for us? Or it is better to interrupt some time?
@ -38,0 +62,4 @@
c.cancel()
return nil
}
if got, want := len(kv.Key), len(cid.ID{})+len(oid.ID{}); got != want {
Owner

len(internalKey{})?

`len(internalKey{})`?
Author
Member

Yeah, this is better, fixed.

Yeah, this is better, fixed.
fyrchik marked this conversation as resolved
@ -38,0 +70,4 @@
c.processed++
obj := objectSDK.New()
var val []byte
val = append(val, kv.Value...)
Owner

val := bytes.Clone (or slice.Copy)?

`val := bytes.Clone` (or `slice.Copy`)?
Author
Member

Updated, I've chosen append because badger uses it in CopyValue.

Updated, I've chosen append because badger uses it in `CopyValue`.
fyrchik marked this conversation as resolved
@ -38,0 +74,4 @@
if err = obj.Unmarshal(val); err == nil {
addr := objectCore.AddressOf(obj)
c.cache.scheduled4FlushMtx.RLock()
_, ok := c.cache.scheduled4Flush[addr]
Owner

Correct me if I am wrong: the purpose of this map is to prevent flushing the same object twice?

Correct me if I am wrong: the purpose of this map is to prevent flushing the same object twice?
Author
Member

Right, It is possible that a background routine which storing data may hang, and on the next iteration the same object will be scheduled. When using stream it is impossible to start from one of the key, no such api. Also, retrieved keys sorted lexicographically, not in putting in db order.

Right, It is possible that a background routine which storing data may hang, and on the next iteration the same object will be scheduled. When using stream it is impossible to start from one of the key, no such api. Also, retrieved keys sorted lexicographically, not in putting in db order.
fyrchik marked this conversation as resolved
@ -38,0 +91,4 @@
return nil
}
} else {
c.cache.log.Debug(fmt.Sprintf("error unmarshal: %s", err))
Owner
  1. If we store garbage data, it should be at least Warn or Error (and probably increase shard error counter
  2. Let's use common scheme with log.Error(logs.Message, zap.Error(err))
1. If we store garbage data, it should be at least `Warn` or `Error` (and probably increase shard error counter 2. Let's use common scheme with `log.Error(logs.Message, zap.Error(err))`
Owner

Actually, if we increase error counter, there is no need to log.

Actually, if we increase error counter, there is no need to log.
Author
Member

Agree, removed log entry.

Agree, removed log entry.
fyrchik marked this conversation as resolved
@ -129,3 +146,1 @@
c.modeMtx.RUnlock()
return
}
ctx, cancel := context.WithCancel(context.TODO())
Owner

Background? Or provide context to flushSmallObjects?

`Background`? Or provide context to `flushSmallObjects`?
Author
Member

Think it is better to use global context in a separate task #642. Requires a lot of refactoring, because we need to change the signature of Init method.

Think it is better to use global context in a separate task #642. Requires a lot of refactoring, because we need to change the signature of `Init` method.
fyrchik marked this conversation as resolved
@ -134,2 +151,2 @@
if count == 0 {
c.modeMtx.RUnlock()
stream := c.db.NewStream()
// Logic within Send method can expect single threaded execution.
Owner

can expect or expects?

`can expect` or `expects`?
Author
Member

All calls to Send are done by a single goroutine.

All calls to `Send` are done by a single goroutine.
fyrchik marked this conversation as resolved
@ -136,0 +151,4 @@
stream := c.db.NewStream()
// Logic within Send method can expect single threaded execution.
stream.Send = coll.Send
if err := stream.Orchestrate(ctx); err != nil {
Owner

Does it exit after all object in the database were streamed through?

Does it exit after all object in the database were streamed through?
Author
Member

Yes, in opposite to Subscribe.

Yes, in opposite to `Subscribe`.
fyrchik marked this conversation as resolved
@ -141,3 +162,2 @@
c.log.Debug(logs.WritecacheTriedToFlushItemsFromWritecache,
zap.Int("count", count),
zap.String("start", base58.Encode(lastKey[:])))
zap.Int("scheduled", coll.scheduled), zap.Int("processed", coll.processed))
Owner

GC interval is 1 minute by default currently. It may be beneficial to run it after each flush cycle instead. What do you think? @TrueCloudLab/storage-core-committers @TrueCloudLab/storage-core-developers

GC interval is 1 minute by default currently. It may be beneficial to run it after each flush cycle instead. What do you think? @TrueCloudLab/storage-core-committers @TrueCloudLab/storage-core-developers
Author
Member

If you mean badgers gs, I think that will be useful here if we scheduled some objects for deletion. We're flushing each second, could this be a problem?

If you mean badgers gs, I think that will be useful here if we scheduled some objects for deletion. We're flushing each second, could this be a problem?

I don't think it must be some correlation between badger GC and flush.

I don't think it must be some correlation between badger GC and flush.
fyrchik marked this conversation as resolved
acid-ant force-pushed bugfix/568-imprv-badger-flush from bcb90c455b to 4c4968f95d 2023-08-24 06:33:11 +00:00 Compare
acid-ant force-pushed bugfix/568-imprv-badger-flush from 4c4968f95d to 7a3c157537 2023-08-24 06:54:40 +00:00 Compare
dstepanov-yadro reviewed 2023-08-24 08:22:38 +00:00
@ -136,0 +153,4 @@
"error during flushing object from wc: %s", err))
}
c.modeMtx.RUnlock()
if coll.scheduled == 0 {

Please explain this line. I don't understand why the flush ends if no objects are scheduled?

Please explain this line. I don't understand why the flush ends if no objects are scheduled?
Author
Member

It is possible that few objects still exists in db and at the same time scheduled for flush.
To prevent iteration over already scheduled objects, we need to check for scheduled counter, not for processed.
This loop running by timer each second.

It is possible that few objects still exists in db and at the same time scheduled for flush. To prevent iteration over already scheduled objects, we need to check for scheduled counter, not for processed. This loop running by timer each second.
dstepanov-yadro marked this conversation as resolved
dstepanov-yadro approved these changes 2023-08-24 09:07:51 +00:00
dstepanov-yadro approved these changes 2023-08-29 07:07:09 +00:00
aarifullin approved these changes 2023-08-29 08:38:22 +00:00
fyrchik reviewed 2023-08-29 08:52:42 +00:00
fyrchik approved these changes 2023-08-29 08:52:56 +00:00
acid-ant force-pushed bugfix/568-imprv-badger-flush from 7a3c157537 to 1129564e7a 2023-08-30 06:33:51 +00:00 Compare
fyrchik merged commit 4dff9555f1 into master 2023-08-30 17:22:32 +00:00
Sign in to join this conversation.
No milestone
No project
No assignees
4 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: TrueCloudLab/frostfs-node#641
No description provided.