refactor: remove sql.Tx from Batch#5080
Conversation
icorderi
force-pushed
the
refactor/store-atomic-3
branch
3 times, most recently
from
f51b8cb to
4957128
Compare
Codecov Report
@@ Coverage Diff @@
## master #5080 +/- ##
==========================================
- Coverage 53.47% 53.31% -0.17%
==========================================
Files 431 431
Lines 54373 54524 +151
==========================================
- Hits 29076 29069 -7
- Misses 23044 23170 +126
- Partials 2253 2285 +32
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
icorderi
force-pushed
the
refactor/store-atomic-3
branch
2 times, most recently
from
73f9855 to
eb3c09b
Compare
algorandskiy
left a comment
algorandskiy
left a comment
There was a problem hiding this comment.
I cannot find anything wrong with this PR although the fact many Create* functions now error and they did not before. This affects not only writing but reading as well so there is possibility for producing different results on upgraded and non-upgraded nodes. Need to re-examine this.
This PR also misses Create -> Make rename as discussed earlier, and two tests migration from ledger to catchpoints
| require.True(t, allAccountsHaveStateProofPKs(accts)) | ||
| dbs.Transaction(func(ctx context.Context, tx store.TransactionScope) (err error) { | ||
| accts := ledgertesting.RandomAccounts(20, false) | ||
| _ = tx.AccountsInitTest(t, accts, protocol.ConsensusCurrentVersion) |
There was a problem hiding this comment.
idk but AccountsInitTest in TransactionScope looks a bit weird abstraction
| func BenchmarkReadingRandomBalancesDisk(b *testing.B) { | ||
| benchmarkReadingRandomBalances(b, false) | ||
| } | ||
| func BenchmarkWritingRandomBalancesDisk(b *testing.B) { |
There was a problem hiding this comment.
suggestion: this is was a basic benchmark showing sqlite performance. Maybe moving to store/sql_test.go or removing as a separate commit/PR
|
|
||
| // AccountsAllTest iterates the account table and returns a map of the data | ||
| // It is meant only for testing purposes - it is heavy and has no production use case. | ||
| func (r *accountsV2Reader) AccountsAllTest() (bals map[basics.Address]basics.AccountData, err error) { |
There was a problem hiding this comment.
nit: since this is part of an interface probably need a better name AccountsAllTest -> GetAllAccountsTesting or similar
There was a problem hiding this comment.
I would suggest we can do a renaming pass on a few methods on a followup PR.
This name just follows the previous name it already had AcountsAll and adds Test to it to flag it as a testing only method.
Making it GetAllAccountTesting would add several naming convention changes.
I do prefer the name, but rather deal with several renames all at once than leave this one on its own.
icorderi
force-pushed
the
refactor/store-atomic-3
branch
from
a744e0b to
641a2e6
Compare
AlgoAxel
left a comment
AlgoAxel
left a comment
There was a problem hiding this comment.
Just took a once over, but as I am also working this branch, I am familiar with it and given the checks passing, am comfortable adding my approval. As I helped in development on this PR, if we'd like another reviewer that's fine, but lots of this work was done by Ignacio before I started adding.
|
|
||
| func makeCatchpointWriter(ctx context.Context, filePath string, tx *sql.Tx, maxResourcesPerChunk int) (*catchpointWriter, error) { | ||
| arw := store.NewAccountsSQLReaderWriter(tx) | ||
| func makeCatchpointWriter(ctx context.Context, filePath string, tx store.TransactionScope, maxResourcesPerChunk int) (*catchpointWriter, error) { |
4 participants
Summary
sql.Txfrom the.Batch(..)callback.sql.Txfrom the.Snapshot(..)callback.This PR is part of a series:
db.Atomicusage #5021sql.Txfrom the transaction callback #5031Test Plan
Existing tests