-
Notifications
You must be signed in to change notification settings - Fork 527
refactor: abstract store and remove all db.Atomic usage
#5021
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
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -51,7 +51,7 @@ var testPoolAddr = basics.Address{0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff | |
| var testSinkAddr = basics.Address{0x2c, 0x2a, 0x6c, 0xe9, 0xa9, 0xa7, 0xc2, 0x8c, 0x22, 0x95, 0xfd, 0x32, 0x4f, 0x77, 0xa5, 0x4, 0x8b, 0x42, 0xc2, 0xb7, 0xa8, 0x54, 0x84, 0xb6, 0x80, 0xb1, 0xe1, 0x3d, 0x59, 0x9b, 0xeb, 0x36} | ||
|
|
||
| type mockLedgerForTracker struct { | ||
| dbs db.Pair | ||
| dbs store.TrackerStore | ||
| blocks []blockEntry | ||
| deltas []ledgercore.StateDelta | ||
| log logging.Logger | ||
|
|
@@ -94,9 +94,8 @@ func setupAccts(niter int) []map[basics.Address]basics.AccountData { | |
| } | ||
|
|
||
| func makeMockLedgerForTrackerWithLogger(t testing.TB, inMemory bool, initialBlocksCount int, consensusVersion protocol.ConsensusVersion, accts []map[basics.Address]basics.AccountData, l logging.Logger) *mockLedgerForTracker { | ||
| dbs, fileName := storetesting.DbOpenTest(t, inMemory) | ||
| dbs.Rdb.SetLogger(l) | ||
| dbs.Wdb.SetLogger(l) | ||
| dbs, fileName := store.DbOpenTrackerTest(t, inMemory) | ||
| dbs.SetLogger(l) | ||
|
|
||
| blocks := randomInitChain(consensusVersion, initialBlocksCount) | ||
| deltas := make([]ledgercore.StateDelta, initialBlocksCount) | ||
|
|
@@ -154,7 +153,7 @@ func (ml *mockLedgerForTracker) fork(t testing.TB) *mockLedgerForTracker { | |
| copy(newLedgerTracker.deltas, ml.deltas) | ||
|
|
||
| // calling Vacuum implies flushing the database content to disk.. | ||
| ml.dbs.Wdb.Vacuum(context.Background()) | ||
| ml.dbs.Vacuum(context.Background()) | ||
| // copy the database files. | ||
| for _, ext := range []string{"", "-shm", "-wal"} { | ||
| bytes, err := os.ReadFile(ml.filename + ext) | ||
|
|
@@ -167,7 +166,7 @@ func (ml *mockLedgerForTracker) fork(t testing.TB) *mockLedgerForTracker { | |
| dbs.Rdb.SetLogger(dblogger) | ||
| dbs.Wdb.SetLogger(dblogger) | ||
|
|
||
| newLedgerTracker.dbs = dbs | ||
| newLedgerTracker.dbs = store.CreateTrackerSQLStore(dbs) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think go prefers Make/New prefix instead of Create for such functions |
||
| return newLedgerTracker | ||
| } | ||
|
|
||
|
|
@@ -220,7 +219,7 @@ func (ml *mockLedgerForTracker) BlockHdr(rnd basics.Round) (bookkeeping.BlockHea | |
| return ml.blocks[int(rnd)].block.BlockHeader, nil | ||
| } | ||
|
|
||
| func (ml *mockLedgerForTracker) trackerDB() db.Pair { | ||
| func (ml *mockLedgerForTracker) trackerDB() store.TrackerStore { | ||
| return ml.dbs | ||
| } | ||
|
|
||
|
|
@@ -264,7 +263,7 @@ func (au *accountUpdates) allBalances(rnd basics.Round) (bals map[basics.Address | |
| return | ||
| } | ||
|
|
||
| err = au.dbs.Rdb.Atomic(func(ctx context.Context, tx *sql.Tx) error { | ||
| err = au.dbs.Snapshot(func(ctx context.Context, tx *sql.Tx) error { | ||
| var err0 error | ||
| bals, err0 = accountsAll(tx) | ||
| return err0 | ||
|
|
@@ -572,7 +571,7 @@ func TestAcctUpdates(t *testing.T) { | |
|
|
||
| // check the account totals. | ||
| var dbRound basics.Round | ||
| err := ml.dbs.Rdb.Atomic(func(ctx context.Context, tx *sql.Tx) (err error) { | ||
| err := ml.dbs.Snapshot(func(ctx context.Context, tx *sql.Tx) (err error) { | ||
| arw := store.NewAccountsSQLReaderWriter(tx) | ||
| dbRound, err = arw.AccountsRound() | ||
| return | ||
|
|
@@ -586,7 +585,7 @@ func TestAcctUpdates(t *testing.T) { | |
|
|
||
| expectedTotals := ledgertesting.CalculateNewRoundAccountTotals(t, updates, rewardsLevels[dbRound], proto, nil, ledgercore.AccountTotals{}) | ||
| var actualTotals ledgercore.AccountTotals | ||
| err = ml.dbs.Rdb.Atomic(func(ctx context.Context, tx *sql.Tx) (err error) { | ||
| err = ml.dbs.Snapshot(func(ctx context.Context, tx *sql.Tx) (err error) { | ||
| arw := store.NewAccountsSQLReaderWriter(tx) | ||
| actualTotals, err = arw.AccountsTotals(ctx, false) | ||
| return | ||
|
|
@@ -1578,14 +1577,14 @@ func BenchmarkLargeMerkleTrieRebuild(b *testing.B) { | |
| i++ | ||
| } | ||
|
|
||
| err := ml.dbs.Wdb.Atomic(func(ctx context.Context, tx *sql.Tx) (err error) { | ||
| err := ml.dbs.Batch(func(ctx context.Context, tx *sql.Tx) (err error) { | ||
| _, _, _, err = accountsNewRound(tx, updates, compactResourcesDeltas{}, nil, nil, proto, basics.Round(1)) | ||
| return | ||
| }) | ||
| require.NoError(b, err) | ||
| } | ||
|
|
||
| err := ml.dbs.Wdb.Atomic(func(ctx context.Context, tx *sql.Tx) (err error) { | ||
| err := ml.dbs.Batch(func(ctx context.Context, tx *sql.Tx) (err error) { | ||
| arw := store.NewAccountsSQLReaderWriter(tx) | ||
| return arw.UpdateAccountsHashRound(ctx, 1) | ||
| }) | ||
|
|
@@ -2352,7 +2351,7 @@ func TestAcctUpdatesResources(t *testing.T) { | |
|
|
||
| err := au.prepareCommit(dcc) | ||
| require.NoError(t, err) | ||
| err = ml.trackers.dbs.Wdb.Atomic(func(ctx context.Context, tx *sql.Tx) (err error) { | ||
| err = ml.trackers.dbs.Batch(func(ctx context.Context, tx *sql.Tx) (err error) { | ||
| arw := store.NewAccountsSQLReaderWriter(tx) | ||
| err = au.commitRound(ctx, tx, dcc) | ||
| if err != nil { | ||
|
|
@@ -2636,7 +2635,7 @@ func auCommitSync(t *testing.T, rnd basics.Round, au *accountUpdates, ml *mockLe | |
|
|
||
| err := au.prepareCommit(dcc) | ||
| require.NoError(t, err) | ||
| err = ml.trackers.dbs.Wdb.Atomic(func(ctx context.Context, tx *sql.Tx) (err error) { | ||
| err = ml.trackers.dbs.Batch(func(ctx context.Context, tx *sql.Tx) (err error) { | ||
| arw := store.NewAccountsSQLReaderWriter(tx) | ||
| err = au.commitRound(ctx, tx, dcc) | ||
| if err != nil { | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -77,11 +77,6 @@ func catchpointStage1Decoder(r io.Reader) (io.ReadCloser, error) { | |
| return snappyReadCloser{snappy.NewReader(r)}, nil | ||
| } | ||
|
|
||
| type catchpointStore interface { | ||
| store.CatchpointWriter | ||
| store.CatchpointReader | ||
| } | ||
|
|
||
| type catchpointTracker struct { | ||
| // dbDirectory is the directory where the ledger and block sql file resides as well as the parent directory for the catchup files to be generated | ||
| dbDirectory string | ||
|
|
@@ -103,8 +98,8 @@ type catchpointTracker struct { | |
| log logging.Logger | ||
|
|
||
| // Connection to the database. | ||
| dbs db.Pair | ||
| catchpointStore catchpointStore | ||
| dbs store.TrackerStore | ||
| catchpointStore store.CatchpointReaderWriter | ||
|
|
||
| // The last catchpoint label that was written to the database. Should always align with what's in the database. | ||
| // note that this is the last catchpoint *label* and not the catchpoint file. | ||
|
|
@@ -216,7 +211,7 @@ func (ct *catchpointTracker) finishFirstStage(ctx context.Context, dbRound basic | |
| } | ||
| } | ||
|
|
||
| f := func(ctx context.Context, tx *sql.Tx) error { | ||
| return ct.dbs.Batch(func(ctx context.Context, tx *sql.Tx) error { | ||
| crw := store.NewCatchpointSQLReaderWriter(tx) | ||
| err := ct.recordFirstStageInfo(ctx, tx, dbRound, totalKVs, totalAccounts, totalChunks, biggestChunkLen) | ||
| if err != nil { | ||
|
|
@@ -225,8 +220,7 @@ func (ct *catchpointTracker) finishFirstStage(ctx context.Context, dbRound basic | |
|
|
||
| // Clear the db record. | ||
| return crw.WriteCatchpointStateUint64(ctx, store.CatchpointStateWritingFirstStageInfo, 0) | ||
| } | ||
| return ct.dbs.Wdb.Atomic(f) | ||
| }) | ||
| } | ||
|
|
||
| // Possibly finish generating first stage catchpoint db record and data file after | ||
|
|
@@ -319,22 +313,25 @@ func (ct *catchpointTracker) recoverFromCrash(dbRound basics.Round) error { | |
| func (ct *catchpointTracker) loadFromDisk(l ledgerForTracker, dbRound basics.Round) (err error) { | ||
| ct.log = l.trackerLog() | ||
| ct.dbs = l.trackerDB() | ||
| ct.catchpointStore = store.NewCatchpointSQLReaderWriter(l.trackerDB().Wdb.Handle) | ||
| ct.catchpointStore, err = l.trackerDB().CreateCatchpointReaderWriter() | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| ct.roundDigest = nil | ||
| ct.catchpointDataWriting = 0 | ||
| // keep these channel closed if we're not generating catchpoint | ||
| ct.catchpointDataSlowWriting = make(chan struct{}, 1) | ||
| close(ct.catchpointDataSlowWriting) | ||
|
|
||
| err = ct.dbs.Wdb.Atomic(func(ctx context.Context, tx *sql.Tx) error { | ||
| err = ct.dbs.Batch(func(ctx context.Context, tx *sql.Tx) error { | ||
| return ct.initializeHashes(ctx, tx, dbRound) | ||
| }) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| ct.accountsq, err = store.AccountsInitDbQueries(ct.dbs.Rdb.Handle) | ||
| ct.accountsq, err = ct.dbs.CreateAccountsReader() | ||
| if err != nil { | ||
| return | ||
| } | ||
|
|
@@ -777,9 +774,9 @@ func (ct *catchpointTracker) createCatchpoint(ctx context.Context, accountsRound | |
| return err | ||
| } | ||
|
|
||
| err = ct.dbs.Wdb.Atomic(func(ctx context.Context, tx *sql.Tx) (err error) { | ||
| err = ct.dbs.Batch(func(ctx context.Context, tx *sql.Tx) (err error) { | ||
| crw := store.NewCatchpointSQLReaderWriter(tx) | ||
| err = ct.recordCatchpointFile(ctx, tx, round, relCatchpointFilePath, fileInfo.Size()) | ||
| err = ct.recordCatchpointFile(ctx, crw, round, relCatchpointFilePath, fileInfo.Size()) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
@@ -1090,7 +1087,7 @@ func (ct *catchpointTracker) generateCatchpointData(ctx context.Context, account | |
| var catchpointWriter *catchpointWriter | ||
| start := time.Now() | ||
| ledgerGeneratecatchpointCount.Inc(nil) | ||
| err = ct.dbs.Rdb.AtomicContext(ctx, func(dbCtx context.Context, tx *sql.Tx) (err error) { | ||
| err = ct.dbs.BatchContext(ctx, func(dbCtx context.Context, tx *sql.Tx) (err error) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in #5583 |
||
| catchpointWriter, err = makeCatchpointWriter(dbCtx, catchpointDataFilePath, tx, ResourcesPerCatchpointFileChunk) | ||
| if err != nil { | ||
| return | ||
|
|
@@ -1213,8 +1210,7 @@ func makeCatchpointDataFilePath(accountsRound basics.Round) string { | |
| // after a successful insert operation to the database, it would delete up to 2 old entries, as needed. | ||
| // deleting 2 entries while inserting single entry allow us to adjust the size of the backing storage and have the | ||
| // database and storage realign. | ||
| func (ct *catchpointTracker) recordCatchpointFile(ctx context.Context, e db.Executable, round basics.Round, relCatchpointFilePath string, fileSize int64) (err error) { | ||
| crw := store.NewCatchpointSQLReaderWriter(e) | ||
| func (ct *catchpointTracker) recordCatchpointFile(ctx context.Context, crw store.CatchpointReaderWriter, round basics.Round, relCatchpointFilePath string, fileSize int64) (err error) { | ||
| if ct.catchpointFileHistoryLength != 0 { | ||
| err = crw.StoreCatchpoint(ctx, round, relCatchpointFilePath, "", fileSize) | ||
| if err != nil { | ||
|
|
@@ -1257,7 +1253,7 @@ func (ct *catchpointTracker) GetCatchpointStream(round basics.Round) (ReadCloseS | |
| ledgerGetcatchpointCount.Inc(nil) | ||
| // TODO: we need to generalize this, check @cce PoC PR, he has something | ||
| // somewhat broken for some KVs.. | ||
| err := ct.dbs.Rdb.Atomic(func(ctx context.Context, tx *sql.Tx) (err error) { | ||
| err := ct.dbs.Snapshot(func(ctx context.Context, tx *sql.Tx) (err error) { | ||
| crw := store.NewCatchpointSQLReaderWriter(tx) | ||
| dbFileName, _, fileSize, err = crw.GetCatchpoint(ctx, round) | ||
| return | ||
|
|
@@ -1277,8 +1273,11 @@ func (ct *catchpointTracker) GetCatchpointStream(round basics.Round) (ReadCloseS | |
| if os.IsNotExist(err) { | ||
| // the database told us that we have this file.. but we couldn't find it. | ||
| // delete it from the database. | ||
| err := ct.recordCatchpointFile( | ||
| context.Background(), ct.dbs.Wdb.Handle, round, "", 0) | ||
| crw, err := ct.dbs.CreateCatchpointReaderWriter() | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| err = ct.recordCatchpointFile(context.Background(), crw, round, "", 0) | ||
| if err != nil { | ||
| ct.log.Warnf("catchpointTracker.GetCatchpointStream() unable to delete missing catchpoint entry: %v", err) | ||
| return nil, err | ||
|
|
@@ -1302,10 +1301,11 @@ func (ct *catchpointTracker) GetCatchpointStream(round basics.Round) (ReadCloseS | |
| // we couldn't get the stat, so just return with the file. | ||
| return &readCloseSizer{ReadCloser: file, size: -1}, nil | ||
| } | ||
|
|
||
| err = ct.recordCatchpointFile( | ||
| context.Background(), ct.dbs.Wdb.Handle, round, relCatchpointFilePath, | ||
| fileInfo.Size()) | ||
| crw, err := ct.dbs.CreateCatchpointReaderWriter() | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| err = ct.recordCatchpointFile(context.Background(), crw, round, relCatchpointFilePath, fileInfo.Size()) | ||
| if err != nil { | ||
| ct.log.Warnf("catchpointTracker.GetCatchpointStream() unable to save missing catchpoint entry: %v", err) | ||
| } | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Snapshot? AccountsOnlineRoundParams is part of
accountsV2ReaderThere 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.
we will sort out snapshot, batch, transaction on the PR I'm work on on right now based on what methods they end up calling.
The PR I'm working on gets rid of that
tx *sql.Txin the callbacks in favor of using interfaces with the methods we have defined already.