Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extract uncurried functions from storage writes #6803

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

zhangchiqing
Copy link
Member

@zhangchiqing zhangchiqing commented Dec 12, 2024

This PR:

  1. Extract uncurried functions from storage writes operations. Useful for refactoring with the uncurried functions
  2. Handle the error closing the iterator
  3. Add the prefix and related functions

@zhangchiqing zhangchiqing changed the base branch from master to leo/db-ops December 12, 2024 18:19
@codecov-commenter
Copy link

codecov-commenter commented Dec 12, 2024

Codecov Report

Attention: Patch coverage is 57.77778% with 38 lines in your changes missing coverage. Please review.

Project coverage is 40.96%. Comparing base (61a0b0e) to head (a0068e1).

Files with missing lines Patch % Lines
storage/operation/prefix.go 0.00% 29 Missing ⚠️
storage/operation/writes.go 62.50% 5 Missing and 4 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6803      +/-   ##
==========================================
- Coverage   40.96%   40.96%   -0.01%     
==========================================
  Files        2093     2096       +3     
  Lines      184478   184516      +38     
==========================================
+ Hits        75577    75590      +13     
- Misses     102579   102608      +29     
+ Partials     6322     6318       -4     
Flag Coverage Δ
unittests 40.96% <57.77%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Base automatically changed from leo/db-ops to master December 12, 2024 18:39
@zhangchiqing zhangchiqing marked this pull request as ready for review December 12, 2024 18:42
// 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 {
Copy link
Member Author

@zhangchiqing zhangchiqing Dec 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Originally these functions are implemented as curried functions in order to follow the same pattern as badger transaction.

However, since we are moving away from badger transaction, it's simpler just removing this additional wrap by using the uncurried functions.

I extracted the uncurried functions and kept the original functions here so that the same logic are reused, as well as the tests.

Useful for refactoring storage modules with UpsertKey instead of Upsert for instance.

Comment on lines 29 to 31
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()
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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()
}
func CloseAndMergeError(closable io.Closer, original error) error {
merr = multierror.Append(merr, closable.Close())
return merr.ErrorOrNil()
}

I'm not 100% but I think this will just work

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

refactored and added test cases.

"github.com/onflow/flow-go/model/flow"
)

const (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where did these constants come from? Is this a unrelated thing?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants