Fix updating storage ID when flushing objects from writecache #739

dstepanov-yadro · 2023-10-13T11:09:34Z

dstepanov-yadro commented

2023-10-13 11:09:34 +00:00

If object marked as GC before flush (by policier for example), then this object will be in writecache forever.
So now logical errors are skipped when updating storage ID, because GC also requires correct storageID to delete object.

Adding an object to the metabase, if it is not there, is not correct. Scenario: the object is still in the cache, but the garbage collector has removed it from the metabase, then we will create an object in metabase that should not be.

Also shard.Delete method was fixed. Now GC will wait (skip) object, if it doesn't exist in blobstore. So GC waits while writecache flushes object to blobstore.

If object marked as GC before flush (by policier for example), then this object will be in writecache forever. So now logical errors are skipped when updating storage ID, because GC also requires correct storageID to delete object. Adding an object to the metabase, if it is not there, is not correct. Scenario: the object is still in the cache, but the garbage collector has removed it from the metabase, then we will create an object in metabase that should not be. Also `shard.Delete` method was fixed. Now GC will wait (skip) object, if it doesn't exist in blobstore. So GC waits while writecache flushes object to blobstore.

dstepanov-yadro force-pushed fix/writecache_flush_inf from b53a2873fe to fa9e825948

2023-10-13 13:14:07 +00:00

Compare

dstepanov-yadro requested review from storage-core-committers 2023-10-13 14:39:13 +00:00

dstepanov-yadro requested review from storage-core-developers 2023-10-13 14:39:14 +00:00

fyrchik reviewed 2023-10-16 12:58:21 +00:00

pkg/local_object_storage/metabase/storage_id.go Outdated

					
				@ -94,3 +96,3 @@

				// UpdateStorageID updates storage descriptor for objects from the blobstor.

				func (db *DB) UpdateStorageID(prm UpdateStorageIDPrm) (res UpdateStorageIDRes, err error) {

				func (db *DB) UpdateStorageID(ctx context.Context, prm UpdateStorageIDPrm) (res UpdateStorageIDRes, err error) {

fyrchik commented

2023-10-16 12:58:18 +00:00

Can we add a test specifically for this behaviour?

dstepanov-yadro commented

2023-10-16 14:00:24 +00:00

Done

Done ![image](/attachments/b289c9a9-350b-4d36-a064-12bba0ab957a)

fyrchik marked this conversation as resolved

pkg/local_object_storage/metabase/storage_id.go Outdated

					
				@ -108,3 +125,3 @@

					err = db.boltDB.Batch(func(tx *bbolt.Tx) error {

						exists, err := db.exists(tx, prm.addr, currEpoch)

						if err == nil && exists || errors.Is(err, ErrObjectIsExpired) {

						if err == nil && exists || errors.As(err, new(logicerr.Logical)) {

fyrchik commented

2023-10-16 12:56:54 +00:00

I see we do this in engine already, but what is the benefit compared to errors.Is?

I see we do this in engine already, but what is the benefit compared to `errors.Is`?

dstepanov-yadro commented

2023-10-16 13:46:09 +00:00

We compare types, not values:

package main

import (
	"errors"
	"fmt"
)

type MyErr struct {
	err error
}

func (e MyErr) Error() string {
	return e.err.Error()
}

func main() {
	err := MyErr{err: fmt.Errorf("error")}
	fmt.Println(errors.Is(err, new(MyErr))) //false
	fmt.Println(errors.Is(err, MyErr{}))    //false
	fmt.Println(errors.As(err, new(MyErr))) //true
}

We compare types, not values: ``` package main import ( "errors" "fmt" ) type MyErr struct { err error } func (e MyErr) Error() string { return e.err.Error() } func main() { err := MyErr{err: fmt.Errorf("error")} fmt.Println(errors.Is(err, new(MyErr))) //false fmt.Println(errors.Is(err, MyErr{})) //false fmt.Println(errors.As(err, new(MyErr))) //true } ```

fyrchik commented

2023-10-16 14:03:32 +00:00

I've missed we do not implement Is for logical type

I've missed we do not implement `Is` for logical type

fyrchik marked this conversation as resolved

dstepanov-yadro force-pushed fix/writecache_flush_inf from 671f3c3d80 to f2437f7ae9

2023-10-16 14:00:40 +00:00

Compare

fyrchik approved these changes 2023-10-16 14:03:43 +00:00

aarifullin approved these changes 2023-10-16 15:58:14 +00:00

fyrchik merged commit f2437f7ae9 into master

2023-10-16 18:45:06 +00:00