From 137e987a4e65ee253da0e16f4bcd602619b73c0d Mon Sep 17 00:00:00 2001 From: Dmitrii Stepanov Date: Wed, 15 Nov 2023 13:12:23 +0300 Subject: [PATCH 1/2] [#655] storage: Drop `LazyHandler` LazyHandler is implemented and used incorrectly. Signed-off-by: Dmitrii Stepanov --- .../blobstor/blobovniczatree/iterate.go | 4 +-- .../blobstor/common/iterate.go | 1 - .../blobstor/fstree/fstree.go | 35 ++++++++----------- .../blobstor/internal/blobstortest/iterate.go | 27 -------------- .../blobstor/memstore/memstore.go | 4 --- .../writecache/writecachebbolt/flush.go | 17 +++------ 6 files changed, 19 insertions(+), 69 deletions(-) diff --git a/pkg/local_object_storage/blobstor/blobovniczatree/iterate.go b/pkg/local_object_storage/blobstor/blobovniczatree/iterate.go index a2afbb8aa..7129e088a 100644 --- a/pkg/local_object_storage/blobstor/blobovniczatree/iterate.go +++ b/pkg/local_object_storage/blobstor/blobovniczatree/iterate.go @@ -53,9 +53,7 @@ func (b *Blobovniczas) Iterate(ctx context.Context, prm common.IteratePrm) (comm StorageID: []byte(p), }) } - return prm.LazyHandler(elem.Address(), func() ([]byte, error) { - return data, err - }) + return nil }) subPrm.DecodeAddresses() diff --git a/pkg/local_object_storage/blobstor/common/iterate.go b/pkg/local_object_storage/blobstor/common/iterate.go index a6f0da26b..1fbb08d32 100644 --- a/pkg/local_object_storage/blobstor/common/iterate.go +++ b/pkg/local_object_storage/blobstor/common/iterate.go @@ -15,7 +15,6 @@ type IterationHandler func(IterationElement) error // IteratePrm groups the parameters of Iterate operation. type IteratePrm struct { Handler IterationHandler - LazyHandler func(oid.Address, func() ([]byte, error)) error IgnoreErrors bool ErrorHandler func(oid.Address, error) error } diff --git a/pkg/local_object_storage/blobstor/fstree/fstree.go b/pkg/local_object_storage/blobstor/fstree/fstree.go index bf3400bcd..06309f53c 100644 --- a/pkg/local_object_storage/blobstor/fstree/fstree.go +++ b/pkg/local_object_storage/blobstor/fstree/fstree.go @@ -188,31 +188,24 @@ func (t *FSTree) iterate(ctx context.Context, depth uint64, curPath []string, pr continue } - if prm.LazyHandler != nil { - err = prm.LazyHandler(addr, func() ([]byte, error) { - return data, err - }) - } else { - if err == nil { - data, err = t.Decompress(data) - } - if err != nil { - if prm.IgnoreErrors { - if prm.ErrorHandler != nil { - return prm.ErrorHandler(addr, err) - } - continue + if err == nil { + data, err = t.Decompress(data) + } + if err != nil { + if prm.IgnoreErrors { + if prm.ErrorHandler != nil { + return prm.ErrorHandler(addr, err) } - return err + continue } - - err = prm.Handler(common.IterationElement{ - Address: addr, - ObjectData: data, - StorageID: []byte{}, - }) + return err } + err = prm.Handler(common.IterationElement{ + Address: addr, + ObjectData: data, + StorageID: []byte{}, + }) if err != nil { return err } diff --git a/pkg/local_object_storage/blobstor/internal/blobstortest/iterate.go b/pkg/local_object_storage/blobstor/internal/blobstortest/iterate.go index 72f107664..c73a663b6 100644 --- a/pkg/local_object_storage/blobstor/internal/blobstortest/iterate.go +++ b/pkg/local_object_storage/blobstor/internal/blobstortest/iterate.go @@ -6,7 +6,6 @@ import ( "testing" "git.frostfs.info/TrueCloudLab/frostfs-node/pkg/local_object_storage/blobstor/common" - oid "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/object/id" "github.com/stretchr/testify/require" ) @@ -30,8 +29,6 @@ func TestIterate(t *testing.T, cons Constructor, min, max uint64) { runTestNormalHandler(t, s, objects) - runTestLazyHandler(t, s, objects) - runTestIgnoreLogicalErrors(t, s, objects) } @@ -62,30 +59,6 @@ func runTestNormalHandler(t *testing.T, s common.Storage, objects []objectDesc) }) } -func runTestLazyHandler(t *testing.T, s common.Storage, objects []objectDesc) { - t.Run("lazy handler", func(t *testing.T) { - seen := make(map[string]func() ([]byte, error)) - - var iterPrm common.IteratePrm - iterPrm.LazyHandler = func(addr oid.Address, f func() ([]byte, error)) error { - seen[addr.String()] = f - return nil - } - - _, err := s.Iterate(context.Background(), iterPrm) - require.NoError(t, err) - require.Equal(t, len(objects), len(seen)) - for i := range objects { - f, ok := seen[objects[i].addr.String()] - require.True(t, ok) - - data, err := f() - require.NoError(t, err) - require.Equal(t, objects[i].raw, data) - } - }) -} - func runTestIgnoreLogicalErrors(t *testing.T, s common.Storage, objects []objectDesc) { t.Run("ignore errors doesn't work for logical errors", func(t *testing.T) { seen := make(map[string]objectDesc) diff --git a/pkg/local_object_storage/blobstor/memstore/memstore.go b/pkg/local_object_storage/blobstor/memstore/memstore.go index 9428f457f..63e1a3e99 100644 --- a/pkg/local_object_storage/blobstor/memstore/memstore.go +++ b/pkg/local_object_storage/blobstor/memstore/memstore.go @@ -154,10 +154,6 @@ func (s *memstoreImpl) Iterate(_ context.Context, req common.IteratePrm) (common if err := req.Handler(elem); err != nil { return common.IterateRes{}, err } - case req.LazyHandler != nil: - if err := req.LazyHandler(elem.Address, func() ([]byte, error) { return elem.ObjectData, nil }); err != nil { - return common.IterateRes{}, err - } default: if !req.IgnoreErrors { return common.IterateRes{}, logicerr.Wrap(fmt.Errorf("(%T) no Handler or LazyHandler set for IteratePrm", s)) diff --git a/pkg/local_object_storage/writecache/writecachebbolt/flush.go b/pkg/local_object_storage/writecache/writecachebbolt/flush.go index 9e46226d1..9efee1dda 100644 --- a/pkg/local_object_storage/writecache/writecachebbolt/flush.go +++ b/pkg/local_object_storage/writecache/writecachebbolt/flush.go @@ -180,20 +180,11 @@ func (c *cache) reportFlushError(msg string, addr string, err error) { func (c *cache) flushFSTree(ctx context.Context, ignoreErrors bool) error { var prm common.IteratePrm prm.IgnoreErrors = ignoreErrors - prm.LazyHandler = func(addr oid.Address, f func() ([]byte, error)) error { - sAddr := addr.EncodeToString() - - data, err := f() - if err != nil { - c.reportFlushError(logs.FSTreeCantReadFile, sAddr, metaerr.Wrap(err)) - if ignoreErrors { - return nil - } - return err - } + prm.Handler = func(e common.IterationElement) error { + sAddr := e.Address.EncodeToString() var obj objectSDK.Object - err = obj.Unmarshal(data) + err := obj.Unmarshal(e.ObjectData) if err != nil { c.reportFlushError(logs.FSTreeCantUnmarshalObject, sAddr, metaerr.Wrap(err)) if ignoreErrors { @@ -202,7 +193,7 @@ func (c *cache) flushFSTree(ctx context.Context, ignoreErrors bool) error { return err } - err = c.flushObject(ctx, &obj, data, writecache.StorageTypeFSTree) + err = c.flushObject(ctx, &obj, e.ObjectData, writecache.StorageTypeFSTree) if err != nil { if ignoreErrors { return nil -- 2.45.3 From 29fe8c41f33bff2668ab0a020a5401a07859fe38 Mon Sep 17 00:00:00 2001 From: Dmitrii Stepanov Date: Wed, 15 Nov 2023 14:18:03 +0300 Subject: [PATCH 2/2] [#655] storage: Drop `ErrorHandler` The only one usage was for logging. Now logging performed by storage anyway. Signed-off-by: Dmitrii Stepanov --- cmd/frostfs-node/config.go | 1 + .../blobstor/blobovniczatree/iterate.go | 14 ++++++++--- .../blobstor/common/iterate.go | 1 - .../blobstor/fstree/fstree.go | 23 ++++++++++++++----- .../blobstor/fstree/option.go | 8 +++++++ pkg/local_object_storage/blobstor/iterate.go | 15 ++++++------ .../blobstor/memstore/memstore.go | 3 --- 7 files changed, 45 insertions(+), 20 deletions(-) diff --git a/cmd/frostfs-node/config.go b/cmd/frostfs-node/config.go index a43b59ab2..3be33a2a1 100644 --- a/cmd/frostfs-node/config.go +++ b/cmd/frostfs-node/config.go @@ -818,6 +818,7 @@ func (c *cfg) getSubstorageOpts(shCfg shardCfg) []blobstor.SubStorage { fstree.WithPerm(sRead.perm), fstree.WithDepth(sRead.depth), fstree.WithNoSync(sRead.noSync), + fstree.WithLogger(c.log), } if c.metricsCollector != nil { fstreeOpts = append(fstreeOpts, diff --git a/pkg/local_object_storage/blobstor/blobovniczatree/iterate.go b/pkg/local_object_storage/blobstor/blobovniczatree/iterate.go index 7129e088a..d2b285c65 100644 --- a/pkg/local_object_storage/blobstor/blobovniczatree/iterate.go +++ b/pkg/local_object_storage/blobstor/blobovniczatree/iterate.go @@ -6,6 +6,7 @@ import ( "path/filepath" "time" + "git.frostfs.info/TrueCloudLab/frostfs-node/internal/logs" "git.frostfs.info/TrueCloudLab/frostfs-node/pkg/local_object_storage/blobovnicza" "git.frostfs.info/TrueCloudLab/frostfs-node/pkg/local_object_storage/blobstor/common" "git.frostfs.info/TrueCloudLab/frostfs-observability/tracing" @@ -13,6 +14,7 @@ import ( "git.frostfs.info/TrueCloudLab/hrw" "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/trace" + "go.uber.org/zap" ) // Iterate iterates over all objects in b. @@ -38,9 +40,11 @@ func (b *Blobovniczas) Iterate(ctx context.Context, prm common.IteratePrm) (comm data, err := b.compression.Decompress(elem.ObjectData()) if err != nil { if prm.IgnoreErrors { - if prm.ErrorHandler != nil { - return prm.ErrorHandler(elem.Address(), err) - } + b.log.Warn(logs.BlobstorErrorOccurredDuringTheIteration, + zap.Stringer("address", elem.Address()), + zap.String("err", err.Error()), + zap.String("storage_id", p), + zap.String("root_path", b.rootPath)) return nil } return fmt.Errorf("could not decompress object data: %w", err) @@ -70,6 +74,10 @@ func (b *Blobovniczas) iterateBlobovniczas(ctx context.Context, ignoreErrors boo blz, err := shBlz.Open() if err != nil { if ignoreErrors { + b.log.Warn(logs.BlobstorErrorOccurredDuringTheIteration, + zap.String("err", err.Error()), + zap.String("storage_id", p), + zap.String("root_path", b.rootPath)) return false, nil } return false, fmt.Errorf("could not open blobovnicza %s: %w", p, err) diff --git a/pkg/local_object_storage/blobstor/common/iterate.go b/pkg/local_object_storage/blobstor/common/iterate.go index 1fbb08d32..a1b8ff047 100644 --- a/pkg/local_object_storage/blobstor/common/iterate.go +++ b/pkg/local_object_storage/blobstor/common/iterate.go @@ -16,7 +16,6 @@ type IterationHandler func(IterationElement) error type IteratePrm struct { Handler IterationHandler IgnoreErrors bool - ErrorHandler func(oid.Address, error) error } // IterateRes groups the resulting values of Iterate operation. diff --git a/pkg/local_object_storage/blobstor/fstree/fstree.go b/pkg/local_object_storage/blobstor/fstree/fstree.go index 06309f53c..d1fe85901 100644 --- a/pkg/local_object_storage/blobstor/fstree/fstree.go +++ b/pkg/local_object_storage/blobstor/fstree/fstree.go @@ -14,10 +14,12 @@ import ( "syscall" "time" + "git.frostfs.info/TrueCloudLab/frostfs-node/internal/logs" "git.frostfs.info/TrueCloudLab/frostfs-node/pkg/local_object_storage/blobstor/common" "git.frostfs.info/TrueCloudLab/frostfs-node/pkg/local_object_storage/blobstor/compression" "git.frostfs.info/TrueCloudLab/frostfs-node/pkg/local_object_storage/util/logicerr" "git.frostfs.info/TrueCloudLab/frostfs-node/pkg/util" + "git.frostfs.info/TrueCloudLab/frostfs-node/pkg/util/logger" "git.frostfs.info/TrueCloudLab/frostfs-observability/tracing" apistatus "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/client/status" cid "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/container/id" @@ -25,6 +27,7 @@ import ( oid "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/object/id" "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/trace" + "go.uber.org/zap" ) type keyLock interface { @@ -41,6 +44,8 @@ func (l *noopKeyLock) Unlock(string) {} type FSTree struct { Info + log *logger.Logger + *compression.Config Depth uint64 DirNameLen int @@ -86,6 +91,7 @@ func New(opts ...Option) *FSTree { metrics: &noopMetrics{}, fileGuard: &noopKeyLock{}, fileCounter: &noopCounter{}, + log: &logger.Logger{Logger: zap.L()}, } for i := range opts { opts[i](f) @@ -145,9 +151,13 @@ func (t *FSTree) Iterate(ctx context.Context, prm common.IteratePrm) (common.Ite func (t *FSTree) iterate(ctx context.Context, depth uint64, curPath []string, prm common.IteratePrm) error { curName := strings.Join(curPath[1:], "") - des, err := os.ReadDir(filepath.Join(curPath...)) + dirPath := filepath.Join(curPath...) + des, err := os.ReadDir(dirPath) if err != nil { if prm.IgnoreErrors { + t.log.Warn(logs.BlobstorErrorOccurredDuringTheIteration, + zap.String("err", err.Error()), + zap.String("directory_path", dirPath)) return nil } return err @@ -182,8 +192,8 @@ func (t *FSTree) iterate(ctx context.Context, depth uint64, curPath []string, pr if err != nil { continue } - - data, err := os.ReadFile(filepath.Join(curPath...)) + path := filepath.Join(curPath...) + data, err := os.ReadFile(path) if err != nil && os.IsNotExist(err) { continue } @@ -193,9 +203,10 @@ func (t *FSTree) iterate(ctx context.Context, depth uint64, curPath []string, pr } if err != nil { if prm.IgnoreErrors { - if prm.ErrorHandler != nil { - return prm.ErrorHandler(addr, err) - } + t.log.Warn(logs.BlobstorErrorOccurredDuringTheIteration, + zap.Stringer("address", addr), + zap.String("err", err.Error()), + zap.String("path", path)) continue } return err diff --git a/pkg/local_object_storage/blobstor/fstree/option.go b/pkg/local_object_storage/blobstor/fstree/option.go index 21d46ac4d..e6c7a2608 100644 --- a/pkg/local_object_storage/blobstor/fstree/option.go +++ b/pkg/local_object_storage/blobstor/fstree/option.go @@ -3,7 +3,9 @@ package fstree import ( "io/fs" + "git.frostfs.info/TrueCloudLab/frostfs-node/pkg/util/logger" utilSync "git.frostfs.info/TrueCloudLab/frostfs-node/pkg/util/sync" + "go.uber.org/zap" ) type Option func(*FSTree) @@ -51,3 +53,9 @@ func WithFileCounter(c FileCounter) Option { f.fileGuard = utilSync.NewKeyLocker[string]() } } + +func WithLogger(l *logger.Logger) Option { + return func(f *FSTree) { + f.log = &logger.Logger{Logger: l.With(zap.String("component", "FSTree"))} + } +} diff --git a/pkg/local_object_storage/blobstor/iterate.go b/pkg/local_object_storage/blobstor/iterate.go index 5a41e4c4f..f213d7547 100644 --- a/pkg/local_object_storage/blobstor/iterate.go +++ b/pkg/local_object_storage/blobstor/iterate.go @@ -40,7 +40,14 @@ func (b *BlobStor) Iterate(ctx context.Context, prm common.IteratePrm) (common.I for i := range b.storage { _, err := b.storage[i].Storage.Iterate(ctx, prm) - if err != nil && !prm.IgnoreErrors { + if err != nil { + if prm.IgnoreErrors { + b.log.Warn(logs.BlobstorErrorOccurredDuringTheIteration, + zap.String("storage_path", b.storage[i].Storage.Path()), + zap.String("storage_type", b.storage[i].Storage.Type()), + zap.String("err", err.Error())) + continue + } return common.IterateRes{}, fmt.Errorf("blobstor iterator failure: %w", err) } } @@ -57,12 +64,6 @@ func IterateBinaryObjects(ctx context.Context, blz *BlobStor, f func(addr oid.Ad return f(elem.Address, elem.ObjectData, elem.StorageID) } prm.IgnoreErrors = true - prm.ErrorHandler = func(addr oid.Address, err error) error { - blz.log.Warn(logs.BlobstorErrorOccurredDuringTheIteration, - zap.Stringer("address", addr), - zap.String("err", err.Error())) - return nil - } _, err := blz.Iterate(ctx, prm) diff --git a/pkg/local_object_storage/blobstor/memstore/memstore.go b/pkg/local_object_storage/blobstor/memstore/memstore.go index 63e1a3e99..cc4f6921a 100644 --- a/pkg/local_object_storage/blobstor/memstore/memstore.go +++ b/pkg/local_object_storage/blobstor/memstore/memstore.go @@ -142,9 +142,6 @@ func (s *memstoreImpl) Iterate(_ context.Context, req common.IteratePrm) (common var err error if elem.ObjectData, err = s.compression.Decompress(elem.ObjectData); err != nil { if req.IgnoreErrors { - if req.ErrorHandler != nil { - return common.IterateRes{}, req.ErrorHandler(elem.Address, err) - } continue } return common.IterateRes{}, logicerr.Wrap(fmt.Errorf("(%T) decompressing data for address %q: %v", s, elem.Address.String(), err)) -- 2.45.3