-
Notifications
You must be signed in to change notification settings - Fork 471
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
ledger: move accountdb to its own package #4428
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First pass
- remove ledger/TestFirstStagePersistence.7233885710903406427 binary files
- maybe call accountdb package trackerdb ?
@@ -370,7 +371,7 @@ func printAccountsDatabase(databaseName string, fileHeader ledger.CatchpointFile | |||
progress++ | |||
acctCount++ | |||
} | |||
_, err = ledger.LoadAllFullAccounts(context.Background(), tx, balancesTable, resourcesTable, acctCb) | |||
_, err = accountdb.LoadAllFullAccounts(context.Background(), tx, balancesTable, resourcesTable, acctCb) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: rewrite the function as iterator class with Next()
and Close()
"github.com/algorand/go-algorand/protocol" | ||
) | ||
|
||
func accountsAll(tx *sql.Tx) (bals map[basics.Address]basics.AccountData, err error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks like can be replaced by the iterator from the suggestion from above.
"github.com/algorand/msgp/msgp" | ||
) | ||
|
||
type EncodedBalanceRecordV5 struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe this file should go into catchpoint
package.
Rationale: it does not to touch db, only local files
ledger/accountdb/lruaccts.go
Outdated
// accountsList contain the list of persistedAccountData, where the front ones are the most "fresh" | ||
// and the ones on the back are the oldest. | ||
accountsList *persistedAccountDataList | ||
// accounts provides fast access to the various elements in the list by using the account address | ||
// accounts provides fast access to the various elements in the list by using the account Address |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Address -> address
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Something annoying that happened was that when using goland refactor, it modified comments that are unrelated to changed variables. Please let me know if you spot any more weird capitalization in the comments and I will fix them.
m.accountsList = newPersistedAccountList().allocateFreeNodes(pendingWrites) | ||
m.accounts = make(map[basics.Address]*persistedAccountDataListNode, pendingWrites) | ||
m.pendingAccounts = make(chan persistedAccountData, pendingWrites) | ||
m.pendingAccounts = make(chan PersistedAccountData, pendingWrites) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a big win would be if we can keep PersistedAccountData internal.
- Usage in tracker/dcc :
updatedPersistedAccounts []persistedAccountData
could be an interface with a single methodLen()
andlruAccounts
might have methodwriteAccounts
taking this interface and casting to[]persistedAccountData
before iterating. persistedData, err = au.accountsq.lookup(addr)
could also be an interface with
Round()
Valid()
GetLedgerCoreAccountData()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Put these interfaces in acctupdates etc, name them UpdatedAccounts, etc
// so that we can update the base account back. | ||
dcc.updatedPersistedAccounts, dcc.updatedPersistedResources, err = accountsNewRound(tx, dcc.compactAccountDeltas, dcc.compactResourcesDeltas, dcc.compactCreatableDeltas, dcc.genesisProto, dbRound+basics.Round(offset)) | ||
dcc.updatedPersistedAccounts, dcc.updatedPersistedResources, err = accountdb.AccountsNewRound(tx, dcc.compactAccountDeltas, dcc.CompactResourcesDeltas, dcc.compactCreatableDeltas, dcc.genesisProto, dbRound+basics.Round(offset)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
type UpdatedAccounts struct {
Data interface{}
Count uint64
}
Codecov Report
@@ Coverage Diff @@
## master #4428 +/- ##
==========================================
- Coverage 55.28% 53.71% -1.57%
==========================================
Files 398 398
Lines 50336 50288 -48
==========================================
- Hits 27829 27013 -816
- Misses 20178 21072 +894
+ Partials 2329 2203 -126
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
// want to ensure that the "real" written value isn't being overridden by the value from the pending accounts. | ||
round basics.Round | ||
} | ||
|
||
func (pad persistedAccountData) Addr() basics.Address { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@algorandskiy possible opportunity for fewer copies / performance — should we make these pointer receivers and then switch to returning references to persistedAccountData as PersistedAccountData implementations?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes. But when I tried it there is a problem with writing pointers into a cache. Having shared copies break some tests and possibly some non-test functionality
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
A few suggestions for followup PRs:
- move
TrackerDB
to its own package, similar to what was done withBlockDB
- create a
catchpoint
package - create a
cache
package
This looks extremely hard to merge after boxes, closing. |
Summary
This PR moves accountdb, tests, LRU caches and all related stuff into its own package.
Some data types were exported as interfaces, others were hidden.
There are three types of DB access functions:
Future work possible as separate PR:
AccountLoadOld
and CoTest Plan
Rely on existing tests