Enable pvtdata store to receive pvtdata hashes during bootstrap #1927
Conversation
manish-sethi
force-pushed
the
populate_pvtdata_store_2_with_UTs
branch
from
1908900 to
e0de473
Compare
Signed-off-by: manish <manish.sethi@gmail.com>
manish-sethi
force-pushed
the
populate_pvtdata_store_2_with_UTs
branch
from
e0de473 to
364ba89
Compare
| }, | ||
| ) | ||
| purgeMgrBuilder := pvtstatepurgemgmt.NewPurgeMgrBuilder(ledgerID, btlPolicy, p.bookkeepingProvider) | ||
|
|
||
| if err = p.dbProvider.ImportFromSnapshot(ledgerID, savepoint, snapshotDir, purgeMgrBuilder); err != nil { | ||
| snapshotDataImporter, err := p.pvtdataStoreProvider.SnapshotDataImporterFor( |
There was a problem hiding this comment.
Nit. Similar to line 284, we could have a NewPvtDataMissingInfoBuilder() here to be explicit about what is being stored in the pvtdatastore during the import phase.
| membershipProvider ledger.MembershipInfoProvider, | ||
| configHistoryRetriever *confighistory.Retriever, | ||
| ) (*SnapshotDataImporter, error) { | ||
|
|
There was a problem hiding this comment.
Nit. Linter does not seem to like this empty line.
| db := p.dbProvider.GetDBHandle(ledgerID) | ||
| batch := db.NewUpdateBatch() | ||
| batch.Put(lastBlockInBootSnapshotKey, encodeLastBlockInBootSnapshotVal(lastBlockInSnapshot)) | ||
| batch.Put(lastCommittedBlkkey, encodeLastCommittedBlockVal(lastBlockInSnapshot)) |
| if encVal == nil || err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
Nit. We are missing a bit of clarity for saving a few lines of code.
| // FetchBootKVHashes returns the KVHashes from the data that was loaded from a snapshot at the time of | ||
| // bootstrapping. This funciton returns an error if the supplied blkNum is greater than the last block | ||
| // number in the booting snapshot | ||
| func (s *Store) FetchBootKVHashes(blkNum, txNum uint64, ns, coll string) (map[string][]byte, error) { |
There was a problem hiding this comment.
Currently, this function is not used in the production code. As it is an exported method, is the goal to make kvledger use this method to check on the reconciled data to identify matching/mismatching hashes?
| require.EqualError(t, err, "unexpected call. Boot KV Hashes are persisted only for the data imported from snapshot") | ||
| }) | ||
|
|
||
| t.Run("committing-old-blocks-pvtdata-deletes-bootKV-hashes", func(t *testing.T) { |
There was a problem hiding this comment.
I expected this test to be present in the reconciler_xxx_test.go. In general, the test for xxx.go is expected to be found in xxx_test.go. Even certain IDE plugins assume that and there are shortcuts to open an alternative file (test or production code). Breaking this tradition is going to introduce some difficult in the future for sure where we need to search for a particular test as it cannot be found in an alternative file. Hence, I would prefer the test to be present in the alternative file. I leave this to you to decide.
| ) | ||
| } | ||
|
|
||
| if err = p.dbProvider.ImportFromSnapshot(ledgerID, savepoint, snapshotDir, purgeMgrBuilder, snapshotDataImporter); err != nil { |
There was a problem hiding this comment.
I assume we will add a ledger level integration test at kvledger/tests to ensure that the hashes are being stored on the pvtdata store.
| ) | ||
| }) | ||
|
|
||
| t.Run("purger-deletes-expired-bootKV-hashes", func(t *testing.T) { |
There was a problem hiding this comment.
Existing test for purger is present in TestStorePurge() (in the alternative file store_test.go) which takes care of deleting eligible, ineligible, and depriritized missing data along with the expiry entry. IMO, it would be good to have all purge related testing in a single place so that we would know where to look for a particular test.
|
Thanks @cendhu - I'll fix some of these nits in the final PR on this topic. |
Signed-off-by: manish manish.sethi@gmail.com
Type of change
Description
As a follow up PR to #1908, this PR
Additional details
In a follow up PR, the changes in the reconciliation will be made to pull the bootKV hashes from the pvtdata store instead of from the blockstore for the data below snapshot height
Related issues
FAB-18033