[Storage Refactor] Refactor Approvals #6868

zhangchiqing · 2025-01-10T18:37:42Z

This PR refactors the approvals storage from badger transaction to badger batch updates.

It is to prepare for switching from badger to pebble.

codecov-commenter · 2025-01-15T16:28:02Z

Codecov Report

Attention: Patch coverage is 67.14286% with 23 lines in your changes missing coverage. Please review.

Project coverage is 41.16%. Comparing base (914f353) to head (5c6f00c).

Files with missing lines	Patch %	Lines
storage/store/approvals.go	77.04%	10 Missing and 4 partials ⚠️
storage/operation/approvals.go	0.00%	8 Missing ⚠️
engine/testutil/nodes.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6868      +/-   ##
==========================================
+ Coverage   41.10%   41.16%   +0.05%     
==========================================
  Files        2127     2129       +2     
  Lines      186321   186390      +69     
==========================================
+ Hits        76586    76725     +139     
+ Misses     103308   103237      -71     
- Partials     6427     6428       +1

Flag	Coverage Δ
unittests	`41.16% <67.14%> (+0.05%)`	⬆️

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.

peterargue

added a few comments about docs, but othewise looks good.

peterargue · 2025-01-16T17:25:19Z

storage/store/approvals.go

+			return fmt.Errorf("could not lookup result approval ID: %w", err)
+		}
+
+		// no approval found, index the approval


should this go after the error conditional completes on line 79?

No, line 79 is when we have found an approval indexed by the chunk, in that case, we need to compare if the stored on is the same as the new one. And we should not store if they are different, because we should not allow a different approval for the same chunk.

peterargue · 2025-01-16T17:28:26Z

storage/store/cache.go

+
+type Cache[K comparable, V any] struct {
+	metrics module.CacheMetrics
+	// nolint:unused


is this needed? looks like limit is used

peterargue · 2025-01-16T17:29:15Z

storage/store/cache.go

+	"github.com/onflow/flow-go/storage"
+)
+
+// nolint:unused


do we need all of these nolint directives? seems like they will become out of date quickly.

peterargue · 2025-01-16T17:31:48Z

storage/store/cache.go

+}
+
+// Get will try to retrieve the resource from cache first, and then from the
+// injected. During normal operations, the following error returns are expected:


what's the injected referring to here?

peterargue · 2025-01-16T17:32:20Z

storage/store/cache.go

+}
+
+// Get will try to retrieve the resource from cache first, and then from the
+// injected. During normal operations, the following error returns are expected:


nit: it's easier to find the error part when it starts on a new line

Suggested change

// injected. During normal operations, the following error returns are expected:

// injected.

// During normal operations, the following error returns are expected:

peterargue · 2025-01-16T17:32:59Z

storage/store/cache.go

+	return resource, nil
+}
+
+func (c *Cache[K, V]) Remove(key K) {


can you add docs for this method too?

AlexHentschel

Thanks Leo, this all look very good. Essentially all of my suggestions are regarding documentation and code aesthetics.

storage/operation/approvals.go

storage/store/approvals.go

AlexHentschel · 2025-02-04T20:55:10Z

storage/store/approvals.go

+			withRetrieve(retrieve)),
+		indexing: new(sync.Mutex),
+	}
+


Suggested change

or maybe inline the return statement in line 33 for compactness

storage/store/approvals.go

AlexHentschel · 2025-02-04T21:37:22Z

storage/store/approvals.go

+// ResultApprovals store is only used within a verification node, where it is
+// assumed that there is never more than one approval per chunk.
+func (r *ResultApprovals) ByChunk(resultID flow.Identifier, chunkIndex uint64) (*flow.ResultApproval, error) {
+	return r.byChunk(r.db.Reader(), resultID, chunkIndex)


would suggest to inline the body of the byChunk method here to avoid unnecessary layers of abstraction. This is the only place where byChunk is used

AlexHentschel · 2025-02-04T21:52:00Z

storage/store/cache_test.go

The test coverage is kind of very minimal 😅. I know that store.cache is a generalization using generics of existing code that has ran since very long time without problems. Though, if you can spare the time, it would be great to improve the test coverage to mission-critical aspects of the cache at least:

Tests confirming that the storage.ErrNotFound is returned in case of cache misses would be good:

flow-go/storage/store/cache.go

Lines 87 to 90 in dd1a8d5

// Get will try to retrieve the resource from cache first, and then from the

// injected. During normal operations, the following error returns are expected:

// - `storage.ErrNotFound` if key is unknown.

func (c *Cache[K, V]) Get(r storage.Reader, key K) (V, error) {

I think it would be good if we verified that unexpected errors from the [storeFunc](https://github.com/onflow/flow-go/blob/dd1a8d5c13a0b4da1a64759c50df5749fd419d0c/storage/store/cache.go#L22-L27) and retrieveFunc functions are passed up and not accidentally wrapped into storage.ErrNotFound

It would be nice to have tests that checks:

if an element is already cached, we don't interact with the database (i.e retrieve is not called)

after a cache miss and then getting an element from the DB, the element is cached

AlexHentschel · 2025-02-04T21:57:16Z

storage/store/approvals_test.go

to me, it looks like you moved the file storage/badger/approvals_test.go and modified it? I think it would be good to also retain the tests for storage/badger/approvals.go since the old badger-version will probably be still in use for quite some time and maybe even be modified.

storage/store/approvals_test.go

storage/operation/approvals.go

AlexHentschel · 2025-02-04T22:22:19Z

Hey, inspired by your PR, I extended the documentation of the storage.ResultApprovals 👉 PR #6976. If you could give take a look, I hope it is quick (documentation changes only). Thanks

Co-authored-by: Alexander Hentschel <[email protected]>

zhangchiqing changed the base branch from master to leo/db-ops-dbstore January 10, 2025 18:38

zhangchiqing force-pushed the leo/db-ops-dbstore branch from 77fb95b to 5b15c09 Compare January 10, 2025 18:39

zhangchiqing force-pushed the leo/storage-refactor-approvals branch from 43bfcd3 to cc06a44 Compare January 10, 2025 18:41

Base automatically changed from leo/db-ops-dbstore to master January 13, 2025 19:55

zhangchiqing force-pushed the leo/storage-refactor-approvals branch from 90051e1 to a4bfe1f Compare January 15, 2025 16:24

zhangchiqing marked this pull request as ready for review January 15, 2025 16:24

zhangchiqing requested a review from a team as a code owner January 15, 2025 16:24

zhangchiqing requested review from fxamacker, janezpodhostnik and peterargue January 15, 2025 16:24

peterargue approved these changes Jan 16, 2025

View reviewed changes

zhangchiqing force-pushed the leo/storage-refactor-approvals branch from a4bfe1f to dd1a8d5 Compare January 28, 2025 18:05

zhangchiqing mentioned this pull request Jan 28, 2025

[Storage Refactor] Using flags to roll out database operation changes for Verification nodes. #6948

Draft

AlexHentschel approved these changes Feb 4, 2025

View reviewed changes

zhangchiqing and others added 11 commits February 5, 2025 10:41

storage update refactor approvals

4001a30

update approvals

78090f1

add approval tests

6913bce

fix approvals

fc816c3

update verificaiton builder

0f60ba7

add comments

01aab9e

revert approvals changes

514226d

Apply suggestions from code review

683f886

Co-authored-by: Alexander Hentschel <[email protected]>

inline approval functions

02e9f80

restore the badger approvals tests

fb0e373

add test case for store/cache

5c6f00c

zhangchiqing force-pushed the leo/storage-refactor-approvals branch from 8c82715 to 5c6f00c Compare February 5, 2025 18:41

zhangchiqing enabled auto-merge February 5, 2025 18:42

zhangchiqing added this pull request to the merge queue Feb 5, 2025

Merged via the queue into master with commit 0aae073 Feb 5, 2025
56 checks passed

zhangchiqing deleted the leo/storage-refactor-approvals branch February 5, 2025 19:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Storage Refactor] Refactor Approvals #6868

[Storage Refactor] Refactor Approvals #6868

zhangchiqing commented Jan 10, 2025 •

edited

Loading

codecov-commenter commented Jan 15, 2025 •

edited

Loading

peterargue left a comment

peterargue Jan 16, 2025

zhangchiqing Jan 28, 2025

peterargue Jan 16, 2025

peterargue Jan 16, 2025

peterargue Jan 16, 2025

peterargue Jan 16, 2025

peterargue Jan 16, 2025

AlexHentschel left a comment

AlexHentschel Feb 4, 2025

AlexHentschel Feb 4, 2025

AlexHentschel Feb 4, 2025

AlexHentschel Feb 4, 2025

AlexHentschel commented Feb 4, 2025 •

edited

Loading

	// injected. During normal operations, the following error returns are expected:
	// injected.
	// During normal operations, the following error returns are expected:

	// Get will try to retrieve the resource from cache first, and then from the
	// injected. During normal operations, the following error returns are expected:
	// - `storage.ErrNotFound` if key is unknown.
	func (c *Cache[K, V]) Get(r storage.Reader, key K) (V, error) {

[Storage Refactor] Refactor Approvals #6868

[Storage Refactor] Refactor Approvals #6868

Conversation

zhangchiqing commented Jan 10, 2025 • edited Loading

codecov-commenter commented Jan 15, 2025 • edited Loading

Codecov Report

peterargue left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexHentschel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexHentschel commented Feb 4, 2025 • edited Loading

zhangchiqing commented Jan 10, 2025 •

edited

Loading

codecov-commenter commented Jan 15, 2025 •

edited

Loading

AlexHentschel commented Feb 4, 2025 •

edited

Loading