ledger: move MakeCatchpointReader back to the Reader interface #5583
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5583 +/- ##
=======================================
Coverage 55.79% 55.79%
=======================================
Files 446 446
Lines 63418 63422 +4
=======================================
+ Hits 35381 35388 +7
Misses 25668 25668
+ Partials 2369 2366 -3
... and 11 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
Now discussing should we keep DB reading for snapshot in |
| @@ -1126,7 +1126,7 @@ func (ct *catchpointTracker) generateCatchpointData(ctx context.Context, account | |||
|
|
|||
| start := time.Now() | |||
| ledgerGeneratecatchpointCount.Inc(nil) | |||
| err = ct.dbs.TransactionContext(ctx, func(dbCtx context.Context, tx trackerdb.TransactionScope) (err error) { | |||
| err = ct.dbs.SnapshotContext(ctx, func(dbCtx context.Context, tx trackerdb.SnapshotScope) (err error) { | |||
There was a problem hiding this comment.
I got kind of lost trying to figure out how things "originally" were implemented and the history of changes, tracking down when things switched from Rdb/Wdb to Snapshot/Transaction — just for reference, jotting down my notes —
this was changed from Rdb to BatchContext (now called TransactionContext) in #5021 in https://github.com/algorand/go-algorand/pull/5021/files#diff-9ca6833a989d244ce6b95aaddf4f2f0889203f7e3e6918b0c9ace37c62f21320R1090
| @@ -1316,7 +1316,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.Transaction(func(ctx context.Context, tx trackerdb.TransactionScope) (err error) { | |||
| err := ct.dbs.Snapshot(func(ctx context.Context, tx trackerdb.SnapshotScope) (err error) { | |||
There was a problem hiding this comment.
This was changed from Rdb to Snapshot in #5021 (which preserved old dbpair behavior) and then from Snapshot to Transaction in #5451 in https://github.com/algorand/go-algorand/pull/5451/files#diff-9ca6833a989d244ce6b95aaddf4f2f0889203f7e3e6918b0c9ace37c62f21320R1319
| @@ -107,7 +107,7 @@ func (data catchpointStateProofVerificationContext) ToBeHashed() (protocol.HashI | |||
| return protocol.StateProofVerCtx, protocol.Encode(&data) | |||
| } | |||
|
|
|||
| func makeCatchpointWriter(ctx context.Context, filePath string, tx trackerdb.TransactionScope, maxResourcesPerChunk int) (*catchpointWriter, error) { | |||
| func makeCatchpointWriter(ctx context.Context, filePath string, tx trackerdb.SnapshotScope, maxResourcesPerChunk int) (*catchpointWriter, error) { | |||
There was a problem hiding this comment.
This (and the struct field tx) was changed from sql.Tx to TransactionScope in #5080 in https://github.com/algorand/go-algorand/pull/5080/files#diff-ea28f4f1f6d1a703b59a0a03c2f71be0614487d17ab17a3da7183beef0e49ae8R94 — we should give catchpointWriter a better name like catchpointFileDumper or catchpointReadDBToFile to make it sound more read-only
| @@ -82,10 +86,7 @@ type Writer interface { | |||
| // we should split these two sets of methods into two separate interfaces | |||
There was a problem hiding this comment.
Do you think it makes sense to do this now, and split CatchpointReader and CatchpointWriter interfaces? or are there still a few places that mix them both to record the FirstStageInfo and stuff like that and we should do it later
There was a problem hiding this comment.
I guess moving the MakeCatchpointReader & the other two methods into Reader is the first step of that, you could add the three methods into a CatchpointReader interface that is embedded into Reader as a tiny step further towards that
algorandskiy
changed the title
MakeCatchpointReader back to the Reader interfaceMakeCatchpointReader back to the Reader interface
algorandskiy
changed the title
MakeCatchpointReader back to the Reader interface
Summary
#5451 introduced a bug, catchpoint reads need to be kept on a separate readonly connection for the node to make forward progress.
This PR adds the required method to the Reader interface for the store, so it becomes available via the Snapshot scope.
There is an implicit contract that the snapshot is going through a separate read-only handle to the sqlite db.
If this were ever to change, this will break again.
Test Plan