[Access] Pebble checkpoint ingestion #4727
Conversation
Codecov ReportAll modified lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4727 +/- ##
==========================================
- Coverage 55.76% 50.90% -4.87%
==========================================
Files 938 544 -394
Lines 86773 52298 -34475
==========================================
- Hits 48391 26623 -21768
+ Misses 34743 23757 -10986
+ Partials 3639 1918 -1721
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
storage/pebble/lookup.go
Outdated
| @@ -128,3 +130,17 @@ func encodedUint64(height uint64) []byte { | |||
| payload := make([]byte, 0, 8) | |||
| return binary.BigEndian.AppendUint64(payload, height) | |||
| } | |||
|
|
|||
| // KeyToRegisterID converts a ledger key into a register ID. | |||
| func keyToRegisterID(key ledger.Key) (flow.RegisterID, error) { | |||
There was a problem hiding this comment.
@janezpodhostnik should this function live here?
We are planning to move all these conversion function somewhere, right?
There was a problem hiding this comment.
Let's reuse the LedgerKeyToRegisterID from this PR https://github.com/onflow/flow-go/pull/4766/files
storage/pebble/bootstrap.go
Outdated
| } | ||
|
|
||
| // IndexCheckpointFile indexes the checkpoint file in the Dir provided as a component | ||
| func (b *Bootstrap) IndexCheckpointFile(ctx irrecoverable.SignalerContext, ready component.ReadyFunc) { |
There was a problem hiding this comment.
can we add some log that we are about to index checkpoint from directory with root height.
storage/pebble/bootstrap.go
Outdated
| } | ||
|
|
||
| // IndexCheckpointFile indexes the checkpoint file in the Dir provided as a component | ||
| func (b *Bootstrap) IndexCheckpointFile() error { |
There was a problem hiding this comment.
better to add some logging in this function to log, the root height, and important steps are done, etc
storage/pebble/bootstrap.go
Outdated
| // check for pre-populated heights, fail if it is populated | ||
| // i.e. the IndexCheckpointFile function has already run for the db in this directory | ||
| checkpointDir, checkpointFileName := filepath.Split(checkpointFile) | ||
| _, _, err := db.Get(latestHeightKey()) |
There was a problem hiding this comment.
Better to reuse the check https://github.com/onflow/flow-go/blob/master/storage/pebble/open.go#L74
…flow/flow-go into amlandeep/pebble-checkpoint-ingestion
| ) (*RegisterBootstrap, error) { | ||
| // check for pre-populated heights, fail if it is populated | ||
| // i.e. the IndexCheckpointFile function has already run for the db in this directory | ||
| isBootstrapped, err := IsBootstrapped(db) |
storage/pebble/bootstrap.go
Outdated
| } | ||
| if isBootstrapped { | ||
| // key detected, attempt to run bootstrap on corrupt or already bootstrapped data | ||
| return nil, fmt.Errorf("found latest key set on badger instance, DB is already bootstrapped") |
There was a problem hiding this comment.
Maybe add an error type above in this file:
ErrAlreadyBootstrapped := errors.New("storage is already bootstrapped and populated with data")And then return this error and in tests better to assert:
assert.ErrorIs(t, err, ErrAlreadyBootstrapped)
storage/pebble/bootstrap_test.go
Outdated
| require.NoError(t, initHeights(p, rootHeight)) | ||
| // errors if FirstHeight or LastHeight are populated | ||
| _, err = NewRegisterBootstrap(p, dir, rootHeight, log) | ||
| require.ErrorContains(t, err, "DB is already bootstrapped") |
There was a problem hiding this comment.
storage/pebble/bootstrap_test.go
Outdated
| fileToDelete := path.Join(dir, fmt.Sprintf("%v.%03d", checkpointFileName, 2)) | ||
| err := os.RemoveAll(fileToDelete) | ||
| require.NoError(t, err) | ||
| pb, _ := createPebbleForTest(t) |
There was a problem hiding this comment.
don't forget to cleanup the dir (otherwise they collect on people's laptops after running tests)
| dbDir := unittest.TempPebblePath(t) | ||
| pb, err := OpenRegisterPebbleDB(dbDir) | ||
| require.NoError(t, err) | ||
| return pb, dbDir |
There was a problem hiding this comment.
optional: consider consolidating all of the calls to close the db and cleanup the dir here.
| return pb, dbDir | |
| t.Cleanup(func() { | |
| require.NoError(t, pb.Close()) | |
| require.NoError(t, os.RemoveAll(dbDir)) | |
| } | |
| return pb, dbDir |
closes #4637