From 8202f87cf154c6a5807bd910222cff70a1e620c9 Mon Sep 17 00:00:00 2001 From: "Leo Zhang (zhangchiqing)" Date: Wed, 11 Dec 2024 15:34:32 -0800 Subject: [PATCH 1/7] extract non-curried writes functions --- storage/operation/prefix.go | 150 +++++++++++++++++++++++++++ storage/operation/writes.go | 64 ++++++------ storage/operation/writes_functors.go | 33 ++++++ 3 files changed, 212 insertions(+), 35 deletions(-) create mode 100644 storage/operation/prefix.go create mode 100644 storage/operation/writes_functors.go diff --git a/storage/operation/prefix.go b/storage/operation/prefix.go new file mode 100644 index 00000000000..19363c9656f --- /dev/null +++ b/storage/operation/prefix.go @@ -0,0 +1,150 @@ +//nolint:golint,unused +package operation + +import ( + "encoding/binary" + "fmt" + + "github.com/onflow/flow-go/model/flow" +) + +const ( + + // codes for special database markers + // codeMax = 1 // deprecated + codeDBType = 2 // specifies a database type + + // codes for views with special meaning + codeSafetyData = 10 // safety data for hotstuff state + codeLivenessData = 11 // liveness data for hotstuff state + + // codes for fields associated with the root state + codeSporkID = 13 + codeProtocolVersion = 14 + codeEpochCommitSafetyThreshold = 15 + codeSporkRootBlockHeight = 16 + + // code for heights with special meaning + codeFinalizedHeight = 20 // latest finalized block height + codeSealedHeight = 21 // latest sealed block height + codeClusterHeight = 22 // latest finalized height on cluster + codeExecutedBlock = 23 // latest executed block with max height + codeFinalizedRootHeight = 24 // the height of the highest finalized block contained in the root snapshot + codeLastCompleteBlockHeight = 25 // the height of the last block for which all collections were received + codeEpochFirstHeight = 26 // the height of the first block in a given epoch + codeSealedRootHeight = 27 // the height of the highest sealed block contained in the root snapshot + + // codes for single entity storage + codeHeader = 30 + _ = 31 // DEPRECATED: 31 was used for identities before epochs + codeGuarantee = 32 + codeSeal = 33 + codeTransaction = 34 + codeCollection = 35 + codeExecutionResult = 36 + codeResultApproval = 37 + codeChunk = 38 + codeExecutionReceiptMeta = 39 // NOTE: prior to Mainnet25, this erroneously had the same value as codeExecutionResult (36) + + // codes for indexing single identifier by identifier/integer + codeHeightToBlock = 40 // index mapping height to block ID + codeBlockIDToLatestSealID = 41 // index mapping a block its last payload seal + codeClusterBlockToRefBlock = 42 // index cluster block ID to reference block ID + codeRefHeightToClusterBlock = 43 // index reference block height to cluster block IDs + codeBlockIDToFinalizedSeal = 44 // index _finalized_ seal by sealed block ID + codeBlockIDToQuorumCertificate = 45 // index of quorum certificates by block ID + codeEpochProtocolStateByBlockID = 46 // index of epoch protocol state entry by block ID + codeProtocolKVStoreByBlockID = 47 // index of protocol KV store entry by block ID + + // codes for indexing multiple identifiers by identifier + codeBlockChildren = 50 // index mapping block ID to children blocks + _ = 51 // DEPRECATED: 51 was used for identity indexes before epochs + codePayloadGuarantees = 52 // index mapping block ID to payload guarantees + codePayloadSeals = 53 // index mapping block ID to payload seals + codeCollectionBlock = 54 // index mapping collection ID to block ID + codeOwnBlockReceipt = 55 // index mapping block ID to execution receipt ID for execution nodes + _ = 56 // DEPRECATED: 56 was used for block->epoch status prior to Dynamic Protocol State in Mainnet25 + codePayloadReceipts = 57 // index mapping block ID to payload receipts + codePayloadResults = 58 // index mapping block ID to payload results + codeAllBlockReceipts = 59 // index mapping of blockID to multiple receipts + codePayloadProtocolStateID = 60 // index mapping block ID to payload protocol state ID + + // codes related to protocol level information + codeEpochSetup = 61 // EpochSetup service event, keyed by ID + codeEpochCommit = 62 // EpochCommit service event, keyed by ID + codeBeaconPrivateKey = 63 // BeaconPrivateKey, keyed by epoch counter + codeDKGStarted = 64 // flag that the DKG for an epoch has been started + codeDKGEnded = 65 // flag that the DKG for an epoch has ended (stores end state) + codeVersionBeacon = 67 // flag for storing version beacons + codeEpochProtocolState = 68 + codeProtocolKVStore = 69 + + // code for ComputationResult upload status storage + // NOTE: for now only GCP uploader is supported. When other uploader (AWS e.g.) needs to + // be supported, we will need to define new code. + codeComputationResults = 66 + + // job queue consumers and producers + codeJobConsumerProcessed = 70 + codeJobQueue = 71 + codeJobQueuePointer = 72 + + // legacy codes (should be cleaned up) + codeChunkDataPack = 100 + codeCommit = 101 + codeEvent = 102 + codeExecutionStateInteractions = 103 + codeTransactionResult = 104 + codeFinalizedCluster = 105 + codeServiceEvent = 106 + codeTransactionResultIndex = 107 + codeLightTransactionResult = 108 + codeLightTransactionResultIndex = 109 + codeTransactionResultErrorMessage = 110 + codeTransactionResultErrorMessageIndex = 111 + codeIndexCollection = 200 + codeIndexExecutionResultByBlock = 202 + codeIndexCollectionByTransaction = 203 + codeIndexResultApprovalByChunk = 204 + + // TEMPORARY codes + blockedNodeIDs = 205 // manual override for adding node IDs to list of ejected nodes, applies to networking layer only + + // internal failure information that should be preserved across restarts + codeExecutionFork = 254 + codeEpochEmergencyFallbackTriggered = 255 +) + +func MakePrefix(code byte, keys ...interface{}) []byte { + prefix := make([]byte, 1) + prefix[0] = code + for _, key := range keys { + prefix = append(prefix, KeyPartToBytes(key)...) + } + return prefix +} + +func KeyPartToBytes(v interface{}) []byte { + switch i := v.(type) { + case uint8: + return []byte{i} + case uint32: + b := make([]byte, 4) + binary.BigEndian.PutUint32(b, i) + return b + case uint64: + b := make([]byte, 8) + binary.BigEndian.PutUint64(b, i) + return b + case string: + return []byte(i) + case flow.Role: + return []byte{byte(i)} + case flow.Identifier: + return i[:] + case flow.ChainID: + return []byte(i) + default: + panic(fmt.Sprintf("unsupported type to convert (%T)", v)) + } +} diff --git a/storage/operation/writes.go b/storage/operation/writes.go index 590526b4686..11b4fdf9d29 100644 --- a/storage/operation/writes.go +++ b/storage/operation/writes.go @@ -10,61 +10,55 @@ import ( "github.com/onflow/flow-go/storage" ) -// Upsert will encode the given entity using msgpack and will insert the resulting +// UpsertByKey will encode the given entity using msgpack and will insert the resulting // binary data under the provided key. // If the key already exists, the value will be overwritten. // Error returns: // - generic error in case of unexpected failure from the database layer or // encoding failure. -func Upsert(key []byte, val interface{}) func(storage.Writer) error { - return func(w storage.Writer) error { - value, err := msgpack.Marshal(val) - if err != nil { - return irrecoverable.NewExceptionf("failed to encode value: %w", err) - } - - err = w.Set(key, value) - if err != nil { - return irrecoverable.NewExceptionf("failed to store data: %w", err) - } +func UpsertByKey(w storage.Writer, key []byte, val interface{}) error { + value, err := msgpack.Marshal(val) + if err != nil { + return irrecoverable.NewExceptionf("failed to encode value: %w", err) + } - return nil + err = w.Set(key, value) + if err != nil { + return irrecoverable.NewExceptionf("failed to store data: %w", err) } + + return nil } -// Remove removes the entity with the given key, if it exists. If it doesn't +// RemoveByKey removes the entity with the given key, if it exists. If it doesn't // exist, this is a no-op. // Error returns: // * generic error in case of unexpected database error -func Remove(key []byte) func(storage.Writer) error { - return func(w storage.Writer) error { - err := w.Delete(key) - if err != nil { - return irrecoverable.NewExceptionf("could not delete item: %w", err) - } - return nil +func RemoveByKey(w storage.Writer, key []byte) error { + err := w.Delete(key) + if err != nil { + return irrecoverable.NewExceptionf("could not delete item: %w", err) } + return nil } -// RemoveByPrefix removes all keys with the given prefix +// RemoveByKeyPrefix removes all keys with the given prefix // Error returns: // * generic error in case of unexpected database error -func RemoveByPrefix(reader storage.Reader, key []byte) func(storage.Writer) error { - return RemoveByRange(reader, key, key) +func RemoveByKeyPrefix(reader storage.Reader, w storage.Writer, key []byte) error { + return RemoveByKeyRange(reader, w, key, key) } -// RemoveByRange removes all keys with a prefix that falls within the range [start, end], both inclusive. +// RemoveByKeyRange removes all keys with a prefix that falls within the range [start, end], both inclusive. // It returns error if endPrefix < startPrefix // no other errors are expected during normal operation -func RemoveByRange(reader storage.Reader, startPrefix []byte, endPrefix []byte) func(storage.Writer) error { - return func(w storage.Writer) error { - if bytes.Compare(startPrefix, endPrefix) > 0 { - return fmt.Errorf("startPrefix key must be less than or equal to endPrefix key") - } - err := w.DeleteByRange(reader, startPrefix, endPrefix) - if err != nil { - return irrecoverable.NewExceptionf("could not delete item: %w", err) - } - return nil +func RemoveByKeyRange(reader storage.Reader, w storage.Writer, startPrefix []byte, endPrefix []byte) error { + if bytes.Compare(startPrefix, endPrefix) > 0 { + return fmt.Errorf("startPrefix key must be less than or equal to endPrefix key") + } + err := w.DeleteByRange(reader, startPrefix, endPrefix) + if err != nil { + return irrecoverable.NewExceptionf("could not delete item: %w", err) } + return nil } diff --git a/storage/operation/writes_functors.go b/storage/operation/writes_functors.go new file mode 100644 index 00000000000..1ee182d040b --- /dev/null +++ b/storage/operation/writes_functors.go @@ -0,0 +1,33 @@ +package operation + +import "github.com/onflow/flow-go/storage" + +// Leo: This package includes deprecated functions that wraps the operation of writing to the database. +// They are needed because the original badger implementation is also implemented in the same wrapped function manner, +// since badger requires writes to be done in a transaction, which is stateful. +// Using these deprecated functions could minimize the changes during refactor and easier to review the changes. +// The simplified implementation of the functions are in the writes.go file, which are encouraged to be used instead. + +func Upsert(key []byte, val interface{}) func(storage.Writer) error { + return func(w storage.Writer) error { + return UpsertByKey(w, key, val) + } +} + +func Remove(key []byte) func(storage.Writer) error { + return func(w storage.Writer) error { + return RemoveByKey(w, key) + } +} + +func RemoveByPrefix(reader storage.Reader, key []byte) func(storage.Writer) error { + return func(w storage.Writer) error { + return RemoveByKeyPrefix(reader, w, key) + } +} + +func RemoveByRange(reader storage.Reader, startPrefix []byte, endPrefix []byte) func(storage.Writer) error { + return func(w storage.Writer) error { + return RemoveByKeyRange(reader, w, startPrefix, endPrefix) + } +} From 1876026e40584f7eaa65126fc6bfc8510bdd6185 Mon Sep 17 00:00:00 2001 From: "Leo Zhang (zhangchiqing)" Date: Thu, 12 Dec 2024 10:11:09 -0800 Subject: [PATCH 2/7] handle closer error --- storage/operation/reads.go | 29 ++++++++++++++++++--------- utils/merr/closer.go | 41 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 61 insertions(+), 9 deletions(-) create mode 100644 utils/merr/closer.go diff --git a/storage/operation/reads.go b/storage/operation/reads.go index 6c14102bbc3..dba9b0835d5 100644 --- a/storage/operation/reads.go +++ b/storage/operation/reads.go @@ -9,6 +9,7 @@ import ( "github.com/onflow/flow-go/module/irrecoverable" "github.com/onflow/flow-go/storage" + "github.com/onflow/flow-go/utils/merr" ) // CheckFunc is a function that checks if the value should be read and decoded. @@ -51,7 +52,7 @@ func IterateKeysByPrefixRange(r storage.Reader, startPrefix []byte, endPrefix [] // IterateKeys will iterate over all entries in the database, where the key starts with a prefixes in // the range [startPrefix, endPrefix] (both inclusive). // No errors expected during normal operations. -func IterateKeys(r storage.Reader, startPrefix []byte, endPrefix []byte, iterFunc IterationFunc, opt storage.IteratorOption) error { +func IterateKeys(r storage.Reader, startPrefix []byte, endPrefix []byte, iterFunc IterationFunc, opt storage.IteratorOption) (errToReturn error) { if len(startPrefix) == 0 { return fmt.Errorf("startPrefix prefix is empty") } @@ -69,7 +70,9 @@ func IterateKeys(r storage.Reader, startPrefix []byte, endPrefix []byte, iterFun if err != nil { return fmt.Errorf("can not create iterator: %w", err) } - defer it.Close() + defer func() { + errToReturn = merr.CloseAndMergeError(it, errToReturn) + }() for it.First(); it.Valid(); it.Next() { item := it.IterItem() @@ -130,7 +133,7 @@ func TraverseByPrefix(r storage.Reader, prefix []byte, iterFunc IterationFunc, o // When this returned function is executed (and only then), it will write into the `keyExists` whether // the key exists. // No errors are expected during normal operation. -func KeyExists(r storage.Reader, key []byte) (bool, error) { +func KeyExists(r storage.Reader, key []byte) (exist bool, errToReturn error) { _, closer, err := r.Get(key) if err != nil { // the key does not exist in the database @@ -140,7 +143,9 @@ func KeyExists(r storage.Reader, key []byte) (bool, error) { // exception while checking for the key return false, irrecoverable.NewExceptionf("could not load data: %w", err) } - defer closer.Close() + defer func() { + errToReturn = merr.CloseAndMergeError(closer, errToReturn) + }() // the key does exist in the database return true, nil @@ -153,13 +158,15 @@ func KeyExists(r storage.Reader, key []byte) (bool, error) { // - storage.ErrNotFound if the key does not exist in the database // - generic error in case of unexpected failure from the database layer, or failure // to decode an existing database value -func RetrieveByKey(r storage.Reader, key []byte, entity interface{}) error { +func RetrieveByKey(r storage.Reader, key []byte, entity interface{}) (errToReturn error) { val, closer, err := r.Get(key) if err != nil { return err } - defer closer.Close() + defer func() { + errToReturn = merr.CloseAndMergeError(closer, errToReturn) + }() err = msgpack.Unmarshal(val, entity) if err != nil { @@ -172,7 +179,7 @@ func RetrieveByKey(r storage.Reader, key []byte, entity interface{}) error { // keys with the format prefix` + `height` (where "+" denotes concatenation of binary strings). The height // is encoded as Big-Endian (entries with numerically smaller height have lexicographically smaller key). // The function finds the *highest* key with the given prefix and height equal to or below the given height. -func FindHighestAtOrBelowByPrefix(r storage.Reader, prefix []byte, height uint64, entity interface{}) error { +func FindHighestAtOrBelowByPrefix(r storage.Reader, prefix []byte, height uint64, entity interface{}) (errToReturn error) { if len(prefix) == 0 { return fmt.Errorf("prefix must not be empty") } @@ -182,7 +189,9 @@ func FindHighestAtOrBelowByPrefix(r storage.Reader, prefix []byte, height uint64 if err != nil { return fmt.Errorf("can not create iterator: %w", err) } - defer it.Close() + defer func() { + errToReturn = merr.CloseAndMergeError(it, errToReturn) + }() var highestKey []byte @@ -203,7 +212,9 @@ func FindHighestAtOrBelowByPrefix(r storage.Reader, prefix []byte, height uint64 return err } - defer closer.Close() + defer func() { + errToReturn = merr.CloseAndMergeError(closer, errToReturn) + }() err = msgpack.Unmarshal(val, entity) if err != nil { diff --git a/utils/merr/closer.go b/utils/merr/closer.go new file mode 100644 index 00000000000..a96efcfd61f --- /dev/null +++ b/utils/merr/closer.go @@ -0,0 +1,41 @@ +package merr + +import ( + "io" + + "github.com/hashicorp/go-multierror" +) + +// CloseAndMergeError close the closable and merge the closeErr with the given err into a multierror +// Note: when using this function in a defer function, don't use as below: +// func XXX() ( +// +// err error, +// ) { +// def func() { +// // bad, because the definition of err might get overwritten +// err = closeAndMergeError(closable, err) +// }() +// +// Better to use as below: +// func XXX() ( +// +// errToReturn error, +// ) { +// def func() { +// // good, because the error to returned is only updated here, and guaranteed to be returned +// errToReturn = closeAndMergeError(closable, errToReturn) +// }() +func CloseAndMergeError(closable io.Closer, err error) error { + var merr *multierror.Error + if err != nil { + merr = multierror.Append(merr, err) + } + + closeError := closable.Close() + if closeError != nil { + merr = multierror.Append(merr, closeError) + } + + return merr.ErrorOrNil() +} From a0068e1c8896d7e8109c96eb90a49d84a2a4005e Mon Sep 17 00:00:00 2001 From: "Leo Zhang (zhangchiqing)" Date: Fri, 13 Dec 2024 16:57:43 -0800 Subject: [PATCH 3/7] simplify and consolidate CloseAndMergeError --- ledger/complete/wal/checkpoint_v6_writer.go | 35 +---------- utils/merr/closer.go | 12 +--- utils/merr/closer_test.go | 64 +++++++++++++++++++++ 3 files changed, 67 insertions(+), 44 deletions(-) create mode 100644 utils/merr/closer_test.go diff --git a/ledger/complete/wal/checkpoint_v6_writer.go b/ledger/complete/wal/checkpoint_v6_writer.go index 5c420b8842d..b72eff4392e 100644 --- a/ledger/complete/wal/checkpoint_v6_writer.go +++ b/ledger/complete/wal/checkpoint_v6_writer.go @@ -19,6 +19,7 @@ import ( "github.com/onflow/flow-go/ledger/complete/mtrie/node" "github.com/onflow/flow-go/ledger/complete/mtrie/trie" utilsio "github.com/onflow/flow-go/utils/io" + "github.com/onflow/flow-go/utils/merr" ) const subtrieLevel = 4 @@ -708,39 +709,7 @@ func decodeSubtrieCount(encoded []byte) (uint16, error) { return binary.BigEndian.Uint16(encoded), nil } -// closeAndMergeError close the closable and merge the closeErr with the given err into a multierror -// Note: when using this function in a defer function, don't use as below: -// func XXX() ( -// -// err error, -// ) { -// def func() { -// // bad, because the definition of err might get overwritten -// err = closeAndMergeError(closable, err) -// }() -// -// Better to use as below: -// func XXX() ( -// -// errToReturn error, -// ) { -// def func() { -// // good, because the error to returned is only updated here, and guaranteed to be returned -// errToReturn = closeAndMergeError(closable, errToReturn) -// }() -func closeAndMergeError(closable io.Closer, err error) error { - var merr *multierror.Error - if err != nil { - merr = multierror.Append(merr, err) - } - - closeError := closable.Close() - if closeError != nil { - merr = multierror.Append(merr, closeError) - } - - return merr.ErrorOrNil() -} +var closeAndMergeError = merr.CloseAndMergeError // withFile opens the file at the given path, and calls the given function with the opened file. // it handles closing the file and evicting the file from Linux page cache. diff --git a/utils/merr/closer.go b/utils/merr/closer.go index a96efcfd61f..6a2306d04d3 100644 --- a/utils/merr/closer.go +++ b/utils/merr/closer.go @@ -27,15 +27,5 @@ import ( // errToReturn = closeAndMergeError(closable, errToReturn) // }() func CloseAndMergeError(closable io.Closer, err error) error { - var merr *multierror.Error - if err != nil { - merr = multierror.Append(merr, err) - } - - closeError := closable.Close() - if closeError != nil { - merr = multierror.Append(merr, closeError) - } - - return merr.ErrorOrNil() + return multierror.Append(err, closable.Close()).ErrorOrNil() } diff --git a/utils/merr/closer_test.go b/utils/merr/closer_test.go new file mode 100644 index 00000000000..c2491a1caff --- /dev/null +++ b/utils/merr/closer_test.go @@ -0,0 +1,64 @@ +package merr + +import ( + "errors" + "fmt" + "testing" + + "github.com/stretchr/testify/require" + + "github.com/onflow/flow-go/storage" +) + +// mockCloser is a mock implementation of io.Closer +type mockCloser struct { + closeError error +} + +// Close is a mock implementation of io.Closer.Close +func (c *mockCloser) Close() error { + return c.closeError +} + +func TestCloseAndMergeError(t *testing.T) { + // Create a mock closer + closer := &mockCloser{} + + // Test case 1: no error + err := CloseAndMergeError(closer, nil) + require.Nil(t, err) + + // Test case 2: only original error + err = CloseAndMergeError(closer, errors.New("original error")) + require.NotNil(t, err) + require.Contains(t, err.Error(), "original error") + + // Test case 3: only close error + closer.closeError = errors.New("close error") + err = CloseAndMergeError(closer, nil) + require.NotNil(t, err) + require.Contains(t, err.Error(), "close error") + + // Test case 4: both original error and close error + err = CloseAndMergeError(closer, errors.New("original error")) + require.NotNil(t, err) + require.Contains(t, err.Error(), "original error") + require.Contains(t, err.Error(), "close error") + + // Test case 5: original error is storage.ErrNotFound + err = CloseAndMergeError(closer, fmt.Errorf("not found error: %w", storage.ErrNotFound)) + require.NotNil(t, err) + require.True(t, errors.Is(err, storage.ErrNotFound)) + + // Test case 6: close error is storage.ErrNotFound + closer.closeError = fmt.Errorf("not found error: %w", storage.ErrNotFound) + err = CloseAndMergeError(closer, nil) + require.NotNil(t, err) + require.True(t, errors.Is(err, storage.ErrNotFound)) + + // Test case 7: error check works with multierror + closer.closeError = fmt.Errorf("exception") + err = CloseAndMergeError(closer, fmt.Errorf("not found error: %w", storage.ErrNotFound)) + require.NotNil(t, err) + require.True(t, errors.Is(err, storage.ErrNotFound)) +} From 522e0f3d33455ce7ce34d814e345734c12c693cc Mon Sep 17 00:00:00 2001 From: Leo Zhang Date: Mon, 16 Dec 2024 15:24:39 -0800 Subject: [PATCH 4/7] Apply suggestions from code review Co-authored-by: Jordan Schalm --- utils/merr/closer_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/utils/merr/closer_test.go b/utils/merr/closer_test.go index c2491a1caff..39f83671e90 100644 --- a/utils/merr/closer_test.go +++ b/utils/merr/closer_test.go @@ -30,7 +30,7 @@ func TestCloseAndMergeError(t *testing.T) { // Test case 2: only original error err = CloseAndMergeError(closer, errors.New("original error")) - require.NotNil(t, err) + require.Error(t, err) require.Contains(t, err.Error(), "original error") // Test case 3: only close error @@ -48,7 +48,7 @@ func TestCloseAndMergeError(t *testing.T) { // Test case 5: original error is storage.ErrNotFound err = CloseAndMergeError(closer, fmt.Errorf("not found error: %w", storage.ErrNotFound)) require.NotNil(t, err) - require.True(t, errors.Is(err, storage.ErrNotFound)) + require.ErrorIs(t, err, storage.ErrNotFound) // Test case 6: close error is storage.ErrNotFound closer.closeError = fmt.Errorf("not found error: %w", storage.ErrNotFound) From f285e90f123f3aab4ca5862788fd80763fa515de Mon Sep 17 00:00:00 2001 From: Leo Zhang Date: Mon, 16 Dec 2024 15:26:06 -0800 Subject: [PATCH 5/7] Apply suggestions from code review Co-authored-by: Jordan Schalm --- utils/merr/closer.go | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/utils/merr/closer.go b/utils/merr/closer.go index 6a2306d04d3..a10d3be84fe 100644 --- a/utils/merr/closer.go +++ b/utils/merr/closer.go @@ -9,20 +9,18 @@ import ( // CloseAndMergeError close the closable and merge the closeErr with the given err into a multierror // Note: when using this function in a defer function, don't use as below: // func XXX() ( -// // err error, // ) { -// def func() { -// // bad, because the definition of err might get overwritten +// defer func() { +// // bad, because the definition of err might get overwritten by another deferred function // err = closeAndMergeError(closable, err) // }() // // Better to use as below: // func XXX() ( -// // errToReturn error, // ) { -// def func() { +// defer func() { // // good, because the error to returned is only updated here, and guaranteed to be returned // errToReturn = closeAndMergeError(closable, errToReturn) // }() From 39b4531722800cdac89b3508e4ec2eec4560889e Mon Sep 17 00:00:00 2001 From: "Leo Zhang (zhangchiqing)" Date: Mon, 16 Dec 2024 15:36:58 -0800 Subject: [PATCH 6/7] use assert.Error --- utils/merr/closer_test.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/utils/merr/closer_test.go b/utils/merr/closer_test.go index 39f83671e90..61015bbc8d6 100644 --- a/utils/merr/closer_test.go +++ b/utils/merr/closer_test.go @@ -36,29 +36,29 @@ func TestCloseAndMergeError(t *testing.T) { // Test case 3: only close error closer.closeError = errors.New("close error") err = CloseAndMergeError(closer, nil) - require.NotNil(t, err) + require.Error(t, err) require.Contains(t, err.Error(), "close error") // Test case 4: both original error and close error err = CloseAndMergeError(closer, errors.New("original error")) - require.NotNil(t, err) + require.Error(t, err) require.Contains(t, err.Error(), "original error") require.Contains(t, err.Error(), "close error") // Test case 5: original error is storage.ErrNotFound err = CloseAndMergeError(closer, fmt.Errorf("not found error: %w", storage.ErrNotFound)) - require.NotNil(t, err) + require.Error(t, err) require.ErrorIs(t, err, storage.ErrNotFound) // Test case 6: close error is storage.ErrNotFound closer.closeError = fmt.Errorf("not found error: %w", storage.ErrNotFound) err = CloseAndMergeError(closer, nil) - require.NotNil(t, err) + require.Error(t, err) require.True(t, errors.Is(err, storage.ErrNotFound)) // Test case 7: error check works with multierror closer.closeError = fmt.Errorf("exception") err = CloseAndMergeError(closer, fmt.Errorf("not found error: %w", storage.ErrNotFound)) - require.NotNil(t, err) + require.Error(t, err) require.True(t, errors.Is(err, storage.ErrNotFound)) } From 36ae5ce8e5608955f18e640583323bb7fd893cde Mon Sep 17 00:00:00 2001 From: "Leo Zhang (zhangchiqing)" Date: Mon, 16 Dec 2024 15:55:50 -0800 Subject: [PATCH 7/7] lint --- utils/merr/closer.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/utils/merr/closer.go b/utils/merr/closer.go index a10d3be84fe..bb59c143b47 100644 --- a/utils/merr/closer.go +++ b/utils/merr/closer.go @@ -9,6 +9,7 @@ import ( // CloseAndMergeError close the closable and merge the closeErr with the given err into a multierror // Note: when using this function in a defer function, don't use as below: // func XXX() ( +// // err error, // ) { // defer func() { @@ -18,6 +19,7 @@ import ( // // Better to use as below: // func XXX() ( +// // errToReturn error, // ) { // defer func() {