Migrate-by-account changes #5128

janezpodhostnik · 2023-12-11T17:21:27Z

Not all of this is in use yet, but its extracted from a larger PR #4633 so that it can be reviewed separately.

The most notable changes are:

changes to the Account based migrator so that once the account grouping is done multiple migrators can be used to migrate one account
grouping is done via sorting which speeds up the grouping
introduced a partial runtime interface implementation so it can be used in the migrations

codecov-commenter · 2023-12-11T17:27:27Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (38484eb) 56.49% compared to head (1833916) 63.57%.
Report is 85 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5128      +/-   ##
==========================================
+ Coverage   56.49%   63.57%   +7.08%     
==========================================
  Files         980       13     -967     
  Lines       93192     1650   -91542     
==========================================
- Hits        52649     1049   -51600     
+ Misses      36640      518   -36122     
+ Partials     3903       83    -3820

Flag	Coverage Δ
unittests	`63.57% <ø> (+7.08%)`	⬆️

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.

fxamacker

Great work! 🙌 Thanks for using parallel sort to group accounts!

I focused on non-test code and left some comments. Some tweaks can reduce memory usage by gigabytes and improve speed.

cmd/util/ledger/util/migration_runtime_interface.go

cmd/util/ledger/util/util.go

cmd/util/ledger/migrations/storage_used_migration.go

cmd/util/ledger/migrations/account_based_migration.go

fxamacker

Thanks for updating! I focused on non-test code and left some comments. One suggestion would prevent problems and the other suggestions are minor.

fxamacker · 2023-12-12T22:17:29Z

cmd/util/ledger/util/util.go

+}
+
+func (p *PayloadsReadonlyLedger) GetValue(owner, key []byte) (value []byte, err error) {
+	v, err := p.Snapshot.Get(flow.NewRegisterID(string(owner), string(key)))


Do we need to convert owner to Address here?

Suggested change

v, err := p.Snapshot.Get(flow.NewRegisterID(string(owner), string(key)))

v, err := p.Snapshot.Get(flow.NewRegisterID(string(flow.BytesToAddress(owner).Bytes()), string(key)))

fxamacker · 2023-12-12T22:20:03Z

cmd/util/ledger/util/util.go

+}
+
+func (p *PayloadsReadonlyLedger) ValueExists(owner, key []byte) (exists bool, err error) {
+	_, ok := p.Snapshot.Payloads[flow.NewRegisterID(string(owner), string(key))]


Do we need to convert owner to Address here?

Suggested change

_, ok := p.Snapshot.Payloads[flow.NewRegisterID(string(owner), string(key))]

_, ok := p.Snapshot.Payloads[flow.NewRegisterID(string(flow.BytesToAddress(owner).Bytes()), string(key))]

cmd/util/ledger/util/payload_grouping.go

cmd/util/ledger/migrations/storage_used_migration.go

cmd/util/ledger/migrations/account_based_migration.go

zhangchiqing · 2023-12-13T16:50:10Z

cmd/util/ledger/migrations/account_based_migration.go

+	log.Info().
+		Int("account_count", accountGroups.Len()).
+		Int("payload_count", len(allPayloads)).
+		Msgf("finished migrating Payloads")


do we want to log the total duration here?

The outer framework already does that.

cmd/util/ledger/migrations/account_based_migration.go

zhangchiqing · 2023-12-13T16:57:57Z

cmd/util/ledger/migrations/account_based_migration.go

+						continue
+					}
+
+					if _, ok := knownProblematicAccounts[job.Address]; ok {


Do we need some statistic for how many problematic account we actually see?

Such as a log shows at the end saying, "seen 90 problemetic accounts among the 100 knownProblemeticAccounts, total unmigrated payloads: 3020".

This could allow us to capture some mistake, for instance, if it says seen 0 problemetic accounts among the 0 knownProblemetic Accounts, ..., it means we probably specified a wrong knownProblemetic accounts.

This might allow us to monitor the change of these accounts.

We should always see all of them. Not sure if this would help. But I will add a log on the amount of registers.

zhangchiqing · 2023-12-13T17:15:01Z

cmd/util/ledger/migrations/storage_used_migration.go

+func (m *AccountUsageMigrator) MigrateAccount(
+	_ context.Context,
+	address common.Address,
+	payloads []*ledger.Payload,


Just to check is it guaranteed that there is no duplication in the payloads?

that is up to the caller, but they should be unique.

zhangchiqing · 2023-12-13T17:18:00Z

cmd/util/ledger/migrations/storage_used_migration.go

+	same := m.compareUsage(isOldVersionOfStatusRegister, status, actualSize)
+	if same {
+		// there is no problem with the usage, return
+		return payloads, nil


Maybe return a bool to indicate there is no migration for this account. Is it useful to report how many accounts are migrated?

Suggested change

return payloads, nil

return payloads, false, nil

I feel like the individual migration should do this reporting if needed, since a lot of the time all accounts need migration.

cmd/util/ledger/migrations/storage_used_migration.go

zhangchiqing · 2023-12-13T17:39:22Z

cmd/util/ledger/migrations/account_based_migration.go

+			logAccount(1)
+		}
+		if contextDone {
+			break


Is it possible to reach this?

If contextDone is true, it must have called break

the select break only breaks out of the select, not out of the for loop.

zhangchiqing · 2023-12-13T18:06:36Z

cmd/util/ledger/util/payload_grouping_test.go

+	copy(tmp2, payloads)
+
+	groups1 := util.GroupPayloadsByAccount(log, tmp1, 0)
+	groups2 := util.GroupPayloadsByAccount(log, tmp2, runtime.NumCPU())


This test case compare the result with itself, it validates the concurrency behavior, but didn't validate the correctness of the result.

In order to validate the correctness of the result, I think we should also compare with a result generated by using a map without any concurrency.

That is what the groups1 are.
groups2 are with concurrency groups1 are without.

Yeah, group2 tests the validation.

Both group1 and group2 are from the same implementation which uses sort. I'm suggesting to compare with an implementation without sort, such as a simple map.

zhangchiqing

Looks good, just one more comment for adding a check in test case

zhangchiqing · 2024-01-08T16:20:06Z

cmd/util/ledger/util/payload_grouping_test.go

+	groups1 := util.GroupPayloadsByAccount(log, tmp1, 0)
+	groups2 := util.GroupPayloadsByAccount(log, tmp2, runtime.NumCPU())
+
+	groups3 := map[common.Address][]*ledger.Payload{}


For the payloads from the same address, we only compared the length, it's better to check the payloads in the same group are the same. We could do that by turning the payload group from a slice to a map, and check each of the payload in the same group exists in the map.

Suggested change

groups3 := map[common.Address][]*ledger.Payload{}

groups3 := map[common.Address]map[ledger.Path]*ledger.Payload

I added a require.ElementsMatch to check for this

Add migration infrastructure

4a149e9

janezpodhostnik requested review from zhangchiqing and fxamacker December 11, 2023 17:21

janezpodhostnik self-assigned this Dec 11, 2023

janezpodhostnik requested review from ramtinms and AlexHentschel as code owners December 11, 2023 17:21

janezpodhostnik changed the title ~~Add Migration infrastructure~~ Migrate-by-account changes Dec 11, 2023

fxamacker reviewed Dec 12, 2023

View reviewed changes

address review comments

cacba0d

janezpodhostnik force-pushed the janez/migration-infrastructure branch from 88f51cd to cacba0d Compare December 12, 2023 16:22

janezpodhostnik requested a review from fxamacker December 12, 2023 18:11

fxamacker reviewed Dec 13, 2023

View reviewed changes

janezpodhostnik added 3 commits December 13, 2023 16:12

Merge branch 'master' into janez/migration-infrastructure

aa85e12

review comments

d488ae3

fix coment

8a2a096

zhangchiqing reviewed Dec 13, 2023

View reviewed changes

address review comments

3b63724

fxamacker mentioned this pull request Dec 15, 2023

Storage migration for Atree register inlining #4576

Closed

22 tasks

j1010001 mentioned this pull request Dec 19, 2023

Complete Cadence 1.0 and Atree inlining migration onflow/cadence#2983

Closed

21 tasks

janezpodhostnik added 2 commits January 2, 2024 15:22

Merge branch 'master' into janez/migration-infrastructure

aa2f796

merge fix and added test

0596ab0

janezpodhostnik requested review from zhangchiqing and fxamacker January 2, 2024 14:48

fxamacker approved these changes Jan 2, 2024

View reviewed changes

janezpodhostnik enabled auto-merge January 2, 2024 18:59

janezpodhostnik requested a review from sideninja January 3, 2024 17:51

zhangchiqing approved these changes Jan 8, 2024

View reviewed changes

janezpodhostnik added this pull request to the merge queue Jan 8, 2024

janezpodhostnik removed this pull request from the merge queue due to a manual request Jan 8, 2024

add elementsmatch to test

1833916

janezpodhostnik enabled auto-merge January 8, 2024 17:34

janezpodhostnik added this pull request to the merge queue Jan 8, 2024

Merged via the queue into master with commit f413638 Jan 8, 2024
52 checks passed

janezpodhostnik deleted the janez/migration-infrastructure branch January 8, 2024 20:40

fxamacker mentioned this pull request Jan 11, 2024

Atree storage migration #4633

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate-by-account changes #5128

Migrate-by-account changes #5128

janezpodhostnik commented Dec 11, 2023

codecov-commenter commented Dec 11, 2023 •

edited

Loading

fxamacker left a comment

fxamacker left a comment

fxamacker Dec 12, 2023

fxamacker Dec 12, 2023

zhangchiqing Dec 13, 2023

janezpodhostnik Dec 13, 2023

zhangchiqing Dec 13, 2023

janezpodhostnik Dec 13, 2023

zhangchiqing Dec 13, 2023

janezpodhostnik Dec 13, 2023

zhangchiqing Dec 13, 2023

janezpodhostnik Dec 13, 2023

zhangchiqing Dec 13, 2023

janezpodhostnik Dec 13, 2023

zhangchiqing Dec 13, 2023

janezpodhostnik Dec 13, 2023

zhangchiqing Dec 13, 2023

janezpodhostnik Jan 2, 2024

zhangchiqing left a comment

zhangchiqing Jan 8, 2024

janezpodhostnik Jan 8, 2024

	v, err := p.Snapshot.Get(flow.NewRegisterID(string(owner), string(key)))
	v, err := p.Snapshot.Get(flow.NewRegisterID(string(flow.BytesToAddress(owner).Bytes()), string(key)))

	_, ok := p.Snapshot.Payloads[flow.NewRegisterID(string(owner), string(key))]
	_, ok := p.Snapshot.Payloads[flow.NewRegisterID(string(flow.BytesToAddress(owner).Bytes()), string(key))]

	groups3 := map[common.Address][]*ledger.Payload{}
	groups3 := map[common.Address]map[ledger.Path]*ledger.Payload

Migrate-by-account changes #5128

Migrate-by-account changes #5128

Conversation

janezpodhostnik commented Dec 11, 2023

codecov-commenter commented Dec 11, 2023 • edited Loading

Codecov Report

fxamacker left a comment

Choose a reason for hiding this comment

fxamacker 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

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

Choose a reason for hiding this comment

zhangchiqing left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Dec 11, 2023 •

edited

Loading