-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
feat(store/v2): snapshot manager #18458
Conversation
WalkthroughThe changes involve a significant overhaul of the snapshot management system, introducing new interfaces and methods for snapshot creation and restoration, as well as pruning and storage management. The Changes
Related issues
TipsChat with CodeRabbit Bot (
|
@cool-develope your pull request is missing a changelog! |
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.
Review Status
Actionable comments generated: 4
Configuration used: CodeRabbit UI
Files ignored due to filter (1)
- store/go.mod
Files selected for processing (9)
- store/commitment/iavl/tree.go (2 hunks)
- store/commitment/iavl/tree_test.go (2 hunks)
- store/pruning/manager_test.go (1 hunks)
- store/root/store_test.go (1 hunks)
- store/snapshots/helpers_test.go (6 hunks)
- store/snapshots/manager.go (6 hunks)
- store/snapshots/manager_test.go (8 hunks)
- store/snapshots/types/snapshotter.go (1 hunks)
- store/storage/sqlite/db.go (4 hunks)
Additional comments: 42
store/pruning/manager_test.go (2)
34-34: The
NewIavlTree
function is called with an empty string argument beforeiavl.DefaultConfig()
. This empty string argument is likely a placeholder for a parameter that was either removed or refactored. Verify that theNewIavlTree
function's signature and the expected arguments are correct and that this change does not introduce any unintended behavior.36-36: The
Manager
struct is being initialized withnoopLog
,ss
, andsc
as arguments. Ensure that theNewManager
function's signature has been updated to accept these arguments and that theManager
struct's fields are correctly assigned within theNewManager
function.store/root/store_test.go (1)
- 30-36: The
SetupTest
function has been updated to include thedefaultStoreKey
parameter when initializing thesc
variable. This change should be verified to ensure that it aligns with the intended use ofdefaultStoreKey
and that it does not introduce any unintended side effects in the test suite or theIavlTree
initialization.Overall, the changes in the test suite seem to be in response to the updated interfaces and functionality of the store component. It is important to ensure that all tests pass and that the new functionality works as expected, especially with regards to the snapshot manager feature and the backward compatibility with the old snapshot format.
store/snapshots/types/snapshotter.go (1)
- 2-23: The interfaces
CommitSnapshotter
andStorageSnapshotter
have been clearly defined with their respective responsibilities. TheSnapshot
method inCommitSnapshotter
now correctly takes aversion
argument, which is more generic and can be applied to different types of stores, not just those that use a height-based versioning system. TheRestore
method inCommitSnapshotter
also appropriately takes a channel for storage key-value pairs, which allows for asynchronous and potentially concurrent restoration of state. The removal ofPruneSnapshotHeight
andSetSnapshotInterval
functions suggests that snapshot management is being refactored to a different part of the system or being handled differently, which should be verified for compatibility with existing systems.25-:
TheExtensionSnapshotter
interface remains unchanged in this hunk. It's important to ensure that the unchanged methods still align with the new snapshotting strategy and that any implementations of this interface are updated accordingly if needed.store/commitment/iavl/tree_test.go (6)
3-15: The addition of new imports suggests that new dependencies are being used in the test file. Ensure that these dependencies are properly managed and that the import paths are correct.
17-21: The
generateTree
function has been modified to accept astoreKey
parameter. This change should be reflected wherever the function is called to ensure compatibility.98-156: The new test function
TestSnapshotter
is added to test the snapshot creation and restoration functionality. It is important to ensure that this test covers all relevant cases and that the snapshot and restore operations are working as expected. The use of channels and goroutines should be carefully reviewed to avoid data races.131-131: The
generateTree
function is called with an empty string for thestoreKey
parameter. This might be intentional for testing purposes, but it's important to ensure that this is the expected behavior and that thestoreKey
is not required to be non-empty for theSnapshot
andRestore
methods to function correctly.132-142: The use of a goroutine to write to the
chunks
channel and the subsequent reading from this channel instreamReader
could potentially lead to data races if not properly synchronized. Ensure that the channel is closed after all writes are completed to prevent deadlocks or reading from a closed channel.147-155: The
Restore
method is tested here. It is crucial to ensure that theRestore
method's error handling is robust, especially considering that it reads from a channel which could be closed or could contain unexpected data.store/storage/sqlite/db.go (2)
11-15: The import block is clean and well-organized.
44-51: The
Database
struct correctly implements thestore.VersionedDatabase
andsnapshottypes.StorageSnapshotter
interfaces, as indicated by thevar _ Interface = (*Struct)(nil)
pattern. This is a common Go idiom to ensure at compile time that the struct satisfies the interface.store/snapshots/manager_test.go (8)
17-25: The test setup for
TestManager_List
looks correct, and the assertions are appropriate for the functionality being tested. The test checks both the normal operation and the behavior when the manager is busy.40-46: In
TestManager_LoadChunk
, the test setup and assertions are correct. It tests for the presence of an existing chunk, the absence of a non-existing chunk, and the behavior under a busy manager.66-78:
TestManager_Take
correctly sets up amockCommitSnapshotter
with predefined items and tests snapshot creation. The test also checks for error conditions such as creating a snapshot at a lower height than the latest and while another snapshot is being created.83-96: The test checks for successful snapshot creation at a higher height and validates the snapshot's properties. The use of
commitSnapshotter.SnapshotFormat()
to set the format in the expected snapshot is a good practice as it ensures consistency with the actual implementation.111-117:
TestManager_Prune
tests the pruning functionality and checks for errors when a snapshot is being taken. The test setup and assertions are correct.129-137:
TestManager_Restore
sets up the necessary mock snapshotter and extension snapshotter. It tests various error conditions for the restore operation, such as invalid format, no chunks, and chunk/hash mismatches.181-186: The test checks that while a restore is in progress, other operations such as creating a snapshot or pruning should fail. This is an important concurrency test to ensure that the manager's state is correctly handled.
236-245:
TestManager_TakeError
sets up amockErrorCommitSnapshotter
to simulate an error during snapshot creation. The test correctly asserts that an error is returned.store/snapshots/manager.go (5)
35-46: The
Manager
struct has been updated to includecommitSnapshotter
andstorageSnapshotter
fields, and themultistore
field has been removed. This change aligns with the new snapshot interfaces and should be checked for compatibility with existing code that may have used themultistore
field.65-73: Introduction of
defaultStorageChannelBufferSize
as a global variable. This should be reviewed to ensure that the buffer size is appropriate for the expected workload and that it does not introduce memory issues.77-90: The
NewManager
function signature has been updated to acceptcommitSnapshotter
andstorageSnapshotter
parameters. This change requires verification that all calls toNewManager
have been updated accordingly.204-210: The
createSnapshot
function now uses thecommitSnapshotter
to take a snapshot at a given height. It is important to ensure that thecommitSnapshotter
is properly initialized before this function is called to avoid runtime errors.366-385: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [366-419]
The
doRestoreSnapshot
function has been updated to use thestorageSnapshotter
for restoring the storage state. It is crucial to ensure that thestorageSnapshotter
is correctly handling the key-value pairs and that the error handling is robust, especially in the go-routine that performs the restoration.The overall changes to the
Manager
struct and related functions appear to be part of a significant refactor to support a new snapshotting mechanism. It is essential to thoroughly test these changes to ensure that the snapshot creation and restoration processes work as expected and that the new interfaces are correctly implemented. Additionally, the removal of themultistore
field and the introduction ofcommitSnapshotter
andstorageSnapshotter
fields should be carefully documented to inform users of the changes.store/snapshots/helpers_test.go (9)
18-22: The addition of new imports suggests that the test file now depends on additional modules, which is expected given the changes described in the pull request summary. Ensure that these new dependencies are properly managed and versioned.
109-111: The
mockCommitSnapshotter
struct has been introduced to mock the behavior of a commit snapshotter. This is a common testing pattern and seems to be correctly implemented here.113-117: The
Restore
method checks for an unknown format and returns an error if the format is not recognized. This is a good practice for error handling and validation of input data.140-146: The
Snapshot
method correctly iterates over items and writes them using thesnapshottypes.WriteExtensionPayload
function. This is consistent with the expected behavior of a snapshotter.152-158: The
SnapshotFormat
andSupportedFormats
methods return the current format, which is expected behavior for a snapshotter implementation.160-164: The
mockStorageSnapshotter
struct is a stub with a no-opRestore
method. This is acceptable for a mock implementation, but ensure that the actual implementation ofStorageSnapshotter
interface handles the restoration logic as required.166-186: The
mockErrorCommitSnapshotter
struct is designed to simulate errors during snapshot operations. This is useful for testing error handling in the snapshot manager.149-198: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [188-208]
The
setupBusyManager
function sets up a manager that is busy creating a snapshot. This is a good use of goroutines and channels to simulate concurrent operations in tests.
- 215-242: The
hungCommitSnapshotter
type is used to simulate a snapshotter that is "hung" or taking an indefinite amount of time to complete an operation. This is achieved by blocking on a channel read, which can be unblocked by calling theClose
method. This is a good pattern for testing timeout and cancellation behavior.Overall, the changes in the test file align with the new snapshot manager feature and provide mocks and utilities for testing the new functionality. Ensure that all new code paths are covered by tests and that the tests are passing.
store/commitment/iavl/tree.go (8)
20-20: The
IavlTree
struct now implements thesnapshottypes.CommitSnapshotter
interface, which is a significant change to ensure that theIavlTree
can handle snapshot operations. This change should be thoroughly tested to ensure that the implementation is correct and does not introduce any regressions.26-27: The addition of the
storeKey
field to theIavlTree
struct is a notable change. It is important to ensure that this field is properly set and used throughout the codebase, especially in the context of snapshot operations where the store name is critical.31-36: The
NewIavlTree
function now accepts astoreKey
parameter. This change will require updates to all places in the code whereNewIavlTree
is called. It is important to verify that thestoreKey
is correctly passed and handled in all instances to avoid any potential issues with store identification.end hunk 0
<<<<<<< start hunk 1
96-151: The
Snapshot
method has been implemented to create a snapshot of theIavlTree
at a specific version and write it to a protobuf writer. This method should be carefully reviewed to ensure that the snapshot creation process is correct and efficient. The use ofprotoWriter.WriteMsg
to write the snapshot items must be correct to ensure the snapshot can be restored properly. Additionally, the error handling and the use ofdefer
to close the exporter are good practices to prevent resource leaks.154-228: The
Restore
method has been implemented to restore theIavlTree
from a snapshot. This method reads from a protobuf reader and writes key-value pairs to a channel for storage restoration. It is crucial to ensure that the restoration process is accurate and that the error handling is robust. The use of a loop with aswitch
statement to handle different snapshot item types is a common pattern, but it must be ensured that all cases are handled correctly and that the loop exits as expected. Theimporter.Commit
andimporter.Close
calls are critical and should be reviewed to ensure that they are correctly implemented to avoid any data corruption or resource leaks.170-171: The
storeKey
is being set from the snapshot item, which implies that thestoreKey
is expected to be part of the snapshot. This should be verified to ensure that the snapshot format includes the store name and that it is correctly set during the snapshot creation process.202-206: The code is sending a
store.KVPair
to thechStorage
channel, which is part of the restoration process. It is important to ensure that the receiving end of this channel is correctly processing these key-value pairs and that there is proper synchronization to handle concurrent sends if theRestore
method is called concurrently.225-227: After the restoration process, the code attempts to load the version of the tree that was just restored. It is important to verify that this operation is successful and that the tree state is consistent after a restore operation.
end hunk 1
store/snapshots/types/snapshotter.go
Outdated
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.
do we need a types sub-package? Why not just have it all under snapshots
. Generally, I've been trying to avoid the use of a types
package.
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.
may I ask you about the reason? I think types
package is useful to avoid the import cycle
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.
"types" is typically an anti-pattern in Golang. It ends up becoming a dumpster of random unrelated stuff (e.g. types
in the SDK).
I have all interfaces and basic types in the root store
package.
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.
Review Status
Actionable comments generated: 5
Configuration used: CodeRabbit UI
Files selected for processing (3)
- store/commitment/iavl/tree.go (2 hunks)
- store/snapshots/manager.go (6 hunks)
- store/storage/sqlite/db.go (4 hunks)
Additional comments: 11
store/snapshots/manager.go (4)
35-46: The
Manager
struct has been refactored to includecommitSnapshotter
andstorageSnapshotter
fields, and themultistore
field has been removed. This change aligns with the new snapshot manager feature and should be checked for compatibility with existing code that uses theManager
struct.77-90: The
NewManager
function has been updated to acceptcommitSnapshotter
andstorageSnapshotter
parameters. This change should be verified across the codebase to ensure that all instantiations ofManager
are updated accordingly.203-206: The
createSnapshot
function now uses thecommitSnapshotter
to create a snapshot. This change should be tested to ensure that the snapshot creation process works correctly with the newcommitSnapshotter
implementation.362-381: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [362-415]
The
doRestoreSnapshot
function has been updated to use thestorageSnapshotter
for restoring the storage state. It's important to ensure that the error handling and channel synchronization logic are correctly implemented to prevent data races or deadlocks.The overall changes to the
Manager
struct and related functions appear to be in line with the introduction of the new snapshot manager feature. It is crucial to perform thorough testing, especially for the new snapshot and restore functionalities, to ensure that they work as expected and are compatible with the existing snapshot format. Additionally, the removal of themultistore
field and the introduction ofcommitSnapshotter
andstorageSnapshotter
fields should be carefully checked for compatibility with existing code.store/commitment/iavl/tree.go (7)
3-17: The addition of new imports should be verified to ensure they are used within the file and there are no unused imports, which can lead to unnecessary package dependencies.
21-21: The implementation of the
snapshottypes.CommitSnapshotter
interface should be thoroughly tested to ensure that theSnapshot
andRestore
methods work correctly with the existing snapshot format.27-28: The addition of the
storeKey
field to theIavlTree
struct is a significant change. Ensure that all instances ofIavlTree
creation are updated to include this new parameter.94-95: The modification of the
Prune
method to return an error is a change in the method signature that could affect error handling logic in the calling code. Ensure that all calls toPrune
handle the returned error appropriately.97-153: The new
Snapshot
method should be reviewed for correctness and performance. Ensure that the snapshot creation process is efficient and does not introduce performance bottlenecks, especially for large datasets.156-231: The new
Restore
method should be reviewed for correctness, especially the error handling and resource management (e.g., ensuring thatimporter
is closed properly withdefer
).233-235: If the
Close
method is intended to release resources, verify that it does so correctly. If there are no resources to release, consider whether the method is necessary.---existing comments on 103-103---
alexanderbez's suggestion to uselatestVersion := t.GetLatestVersion()
is already implemented in the current code, so no action is needed here.
I will add the abstraction layer to both commitment and storage. It is for multi stores in the commitment and avoiding duplication in the storage. |
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.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (1)
- store/snapshots/manager.go (6 hunks)
Additional comments: 5
store/snapshots/manager.go (5)
35-46: The refactoring of the
Manager
struct to includecommitSnapshotter
andstorageSnapshotter
while removingmultistore
is a significant change. Ensure that all references tomultistore
in the codebase have been updated to use the new fields.166-174: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [166-210]
The
Create
method has been updated to usecommitSnapshotter
for snapshot creation. Ensure that thecommitSnapshotter
is properly initialized before this method is called, and that error handling is robust, especially in the goroutine that generates snapshot chunks.
204-210: The
createSnapshot
method now usescommitSnapshotter.Snapshot
. It's important to ensure that thestreamWriter
is properly closed in all cases to avoid resource leaks. The current implementation seems to handle this correctly withdefer
andCloseWithError
.366-385: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [366-419]
The
doRestoreSnapshot
method has been refactored to use bothcommitSnapshotter
andstorageSnapshotter
. It's crucial to ensure that the error channels and goroutines are managed correctly to prevent deadlocks or missed errors. The current implementation appears to correctly handle channel operations and goroutine synchronization.
- 409-416: The error handling for the
storageSnapshotter.Restore
method within the goroutine should be verified to ensure that any errors are properly propagated and handled. The current implementation seems to correctly use a channel to communicate errors back to the main goroutine.
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.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- store/pruning/manager_test.go (3 hunks)
Additional comments: 3
store/pruning/manager_test.go (3)
26-37: The
SetupTest
function has been updated to use a new logger for theiavl.IavlTree
and theManager
. This change is likely for better test logging and diagnostics. Ensure that the logger used here is appropriate for the test environment and that it does not interfere with the test results or output.47-62: The
TestPruning
function now includes a loop to dynamically generate key-value pairs for each version, which is a good practice for creating more robust tests. However, ensure that thekvCount
variable is set to a representative number of key-value pairs that would be expected in a real-world scenario to ensure the test is meaningful.70-88: The assertions have been updated to reflect the dynamic generation of key-value pairs. The test now checks for the existence of a key at a specific version and the absence of a key at another version, which is a good practice for testing pruning functionality. Ensure that the versions and keys checked are consistent with the expected behavior of the pruning logic.
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.
Nice work and thank you @cool-develope, just some suggestions please take a look.
return fmt.Errorf("failed to get immutable tree for version %d: %w", version, err) | ||
} | ||
|
||
exporter, err := tree.Export() |
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.
Could we please call this itr
or iter
instead of exporter
? exporter
makes it seem like its purpose is to write out content, but really the purpose is to iterate over the exported content.
store/pruning/manager_test.go
Outdated
for i := uint64(0); i < latestVersion; i++ { | ||
version := i + 1 | ||
cs := store.NewChangeset() | ||
cs.Add([]byte("key"), []byte(fmt.Sprintf("value%d", version))) | ||
for j := 0; j < kvCount; j++ { | ||
cs.Add([]byte(fmt.Sprintf("key-%d", j)), []byte(fmt.Sprintf("value-%d-%d", version, j))) |
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.
Won't this cause a conflict of keys written given than they'll go kvCount times but be repeated latestVersion times?
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.
The combination of key
and version
is unique in the storage, it is enough to test pruning.
func (m *hungCommitSnapshotter) Restore( | ||
height uint64, format uint32, protoReader protoio.Reader, chStorage chan<- *store.KVPair, |
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.
Stylistic nit for unused functions parameters to even indicate to outside callers that it is not to be used
func (m *hungCommitSnapshotter) Restore(_ uint64, _ uint32, _ protoio.Reader, _ chan<- *store.KVPair) (...) {
panic("not implemented")
}
store/commitment/iavl/tree.go
Outdated
} | ||
|
||
// NewIavlTree creates a new IavlTree instance. | ||
func NewIavlTree(db dbm.DB, logger log.Logger, cfg *Config) *IavlTree { | ||
func NewIavlTree(db dbm.DB, logger log.Logger, storeKey string, cfg *Config) *IavlTree { |
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.
i thought there was discussion around abstracting storekey away? does this change the direction of that conversation?
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.
I think the expectation is that dbm.DB
is a PrefixDb (for iavl v0 & v1) so the store key prefix is already baked into that. I guess storekey is required for the snapshot API? (still reading this PR).
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.
db dbm.DB
is not required or used in iavl v2 (since it's an interface to an underlying generic KV store). Maybe we should fold this into Config
if we want to support multiple iavl versions in store v2?
Restore(height uint64, format uint32, protoReader protoio.Reader) (SnapshotItem, error) | ||
// CommitSnapshotter defines an API for creating and restoring snapshots of the | ||
// commitment state. | ||
type CommitSnapshotter interface { |
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.
why is there a commitment specific interface here? this seems like an implementation detail that is leaking into an interface
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.
left a few questions, lets discuss this on the working group, seems like a decision was made but there isnt reasoning listed as to why
It looks like IAVL |
store/storage/sqlite/db.go
Outdated
@@ -19,6 +20,9 @@ const ( | |||
reservedStoreKey = "_RESERVED_" | |||
keyLatestHeight = "latest_height" | |||
|
|||
// TODO: it is a random number, need to be tuned | |||
defaultBatchBufferSize = 100000 |
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.
For the best snapshot import performance we should consider tuning the max WAL size to something big, on the order of single digit gigabytes. see: https://www.sqlite.org/pragma.html#pragma_wal_autocheckpoint
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.
also synchronous=off
will boost throughput: https://www.sqlite.org/pragma.html#pragma_synchronous
store/snapshots/types/snapshotter.go
Outdated
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.
"types" is typically an anti-pattern in Golang. It ends up becoming a dumpster of random unrelated stuff (e.g. types
in the SDK).
I have all interfaces and basic types in the root store
package.
store/commitment/store.go
Outdated
} | ||
|
||
for storeKey, tree := range c.multiTrees { | ||
exporter, err := tree.Export(version) |
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.
This looks like an expensive operation being ran in a loop multiple times. Couldn't we extract the exporter right above once? We could invert the operations that iterate over storeKeys and exporter.Next() to iterate N times over exporter.Next() but storeKeys (M) times
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.
we have several trees
64dc3c9
to
0598995
Compare
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.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (24)
- store/changeset.go (1 hunks)
- store/commitment/iavl/exporter.go (1 hunks)
- store/commitment/iavl/importer.go (1 hunks)
- store/commitment/iavl/tree.go (1 hunks)
- store/commitment/iavl/tree_test.go (1 hunks)
- store/commitment/store.go (2 hunks)
- store/commitment/store_test_suite.go (1 hunks)
- store/commitment/tree.go (2 hunks)
- store/kv/branch/store_test.go (3 hunks)
- store/pruning/manager_test.go (3 hunks)
- store/root/store_test.go (2 hunks)
- store/snapshots/chunk.go (2 hunks)
- store/snapshots/helpers_test.go (9 hunks)
- store/snapshots/manager.go (7 hunks)
- store/snapshots/snapshotter.go (1 hunks)
- store/storage/database.go (1 hunks)
- store/storage/pebbledb/db.go (4 hunks)
- store/storage/pebbledb/db_test.go (1 hunks)
- store/storage/rocksdb/db.go (4 hunks)
- store/storage/rocksdb/db_test.go (2 hunks)
- store/storage/sqlite/db.go (4 hunks)
- store/storage/sqlite/db_test.go (4 hunks)
- store/storage/storage_bench_test.go (2 hunks)
- store/storage/store.go (1 hunks)
Additional comments: 67
store/changeset.go (1)
- 6-10: The addition of the
StoreKey
field to theKVPair
struct is consistent with the PR objectives and the provided summaries. This field is intended to be optional for snapshot restore functionality. Ensure that the rest of the codebase that usesKVPair
is aware of this new field and handles it appropriately.store/commitment/iavl/exporter.go (1)
- 12-40: The implementation of the
Exporter
struct and its methodsNext
andClose
aligns with the PR objectives to introduce a snapshot manager and the changes described in the summary. The error handling in theNext
method correctly translates theiavl.ErrorExportDone
tocommitment.ErrorExportDone
, and theClose
method properly delegates the call to the underlyingiavl.Exporter
.store/commitment/iavl/importer.go (1)
- 9-34: The
Importer
struct and its methodsAdd
,Commit
, andClose
are correctly implemented as wrappers around theiavl.Importer
methods. The conversion fromsnapshotstypes.SnapshotIAVLItem
toiavl.ExportNode
in theAdd
method is done properly, ensuring that the fields match between the two types.store/commitment/iavl/tree.go (5)
86-86: The concern raised by odeke-em about returning a nil exporter if there is a non-nil error seems to have been addressed, as the code correctly returns
nil
along with the error.98-98: The concern raised by odeke-em and alexanderbez about returning a nil importer for a non-nil error has been addressed, as the code correctly returns
nil
along with the error.91-93: The
Exporter
type is correctly instantiated and returned, aligning with the naming convention explained by kocubinski in the previous discussion.103-105: The
Importer
type is correctly instantiated and returned, and there are no outstanding concerns from previous reviews regarding this part of the code.77-110: The implementation of the
Export
andImport
methods in theIavlTree
struct appears to be correct and in line with the PR objectives and previous review comments.store/commitment/iavl/tree_test.go (3)
14-27: The addition of
TestCommitterSuite
is a good practice for ensuring that the new snapshot manager functionality is thoroughly tested.30-34: The summary indicates that
generateTree
was renamed, but the code shows it still exists with a modified implementation. This should be clarified in the summary.36-36: The
TestIavlTree
function provides a comprehensive test for theIavlTree
operations, which is crucial for ensuring the integrity of the snapshot manager's functionality.store/commitment/store.go (4)
3-21: The addition of new imports and the implementation of the
CommitSnapshotter
interface by theCommitStore
type align with the PR objectives to introduce a snapshot manager.138-195: The
Snapshot
method correctly checks for version validity and ensures the snapshot version is not greater than the latest version. However, the TODO comment about checking the parallelism of the loop suggests that there may be plans to parallelize this operation. It's important to ensure thread safety and data consistency if parallel execution is implemented in the future.198-276: The
Restore
method appears to be well-implemented with proper error handling and validation checks. The use of a channel (chStorage
) for storage operations is a good practice for concurrent operations, which aligns with the PR's goal of introducing a snapshot manager that works with the existing snapshot format.278-280: The
Close
method properly attempts to close all trees and collects any errors that occur usingerrors.Join
. This is a good practice for resource management and error reporting.store/commitment/store_test_suite.go (2)
- 29-120: The test suite effectively covers the snapshot and restore functionality of the
CommitStore
. It tests the entire process of creating a snapshot, writing an extension metadata, and restoring from the snapshot, ensuring that the restored data matches the original data. The use of channels and goroutines for streaming data is appropriate and the synchronization withsync.WaitGroup
is correctly implemented to wait for the goroutine to finish processing. The test also verifies that the hash of the restored tree matches the original, which is crucial for ensuring data integrity.The test suite seems to be comprehensive and well-structured, with proper error handling and assertions. However, it is important to ensure that the test suite is integrated with the rest of the application's tests and that it is executed as part of the continuous integration process to catch any regressions or issues early on.
- 29-120: While the unit tests provided in this suite are comprehensive, it is important to remember that end-to-end tests are not included and should be added during the integration with the base application to ensure that the snapshot manager works correctly within the full context of the application.
store/commitment/tree.go (4)
12-13: The introduction of
ErrorExportDone
is clear and follows Go conventions for error declaration.22-44: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [15-29]
The extension of the
Tree
interface withExport
andImport
methods aligns with the PR's objective to introduce snapshot management features.
31-36: The
Exporter
interface is correctly defined with theNext
method, which is expected to iterate over items to be exported.38-44: The
Importer
interface is correctly defined withAdd
andCommit
methods to facilitate the import of snapshot items.store/kv/branch/store_test.go (3)
8-13: The addition of the
"cosmossdk.io/store/v2/storage"
import aligns with the changes made in theSetupTest
function to use the newstorage
package.28-31: The
SetupTest
function has been updated to use the newstorage
package, which is consistent with the PR objectives to introduce a new snapshot manager and related functionalities.41-47: The
ApplyChangeset
method is now called on thess
variable, which is an instance ofStorageStore
, and thekvStore
is initialized withss
. This reflects the changes made to use the newstorage
package.store/pruning/manager_test.go (3)
11-15:
The addition of new imports for"cosmossdk.io/store/v2/storage"
and"cosmossdk.io/store/v2/storage/sqlite"
is consistent with the PR objectives to support the new snapshot manager feature.38-43:
The renaming ofss
tosqliteDB
and the creation of a newstorage.NewStorageStore
instance are in line with the PR's goal to implement a snapshotter API for storage stores.63-66:
The comment change from "write 10 batches" to "write batches" is a minor clarification and does not impact the test's logic.store/root/store_test.go (1)
- 37-37: Verify that
defaultStoreKey
is defined and used consistently across the codebase.
Verification result:
The
defaultStoreKey
is properly defined instore/pruning/manager_test.go
andstore/root/store.go
and is used consistently across the codebase in various test files and the main store implementation.store/snapshots/chunk.go (3)
- 6-9: The import path
snapshottypes
has been correctly renamed tosnapshotstypes
. Ensure that all references to the old import path are updated throughout the entire codebase.
Verification result:
The import path change from
snapshottypes
tosnapshotstypes
has been successfully reflected in the codebase. The search for the old import path returned no results, indicating that it has been fully replaced. The new import path is correctly used in multiple files, as verified by the search results.
170-182: The function
ValidRestoreHeight
correctly uses the renamed importsnapshotstypes
. This change is consistent with the PR objectives and the summary.169-183: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [1-183]
No further issues found in the provided file. Ensure that the renaming of the import path is reflected across all files that may use it.
Verification result:
The renaming of the import path from
snapshottypes
tosnapshotstypes
has been successfully reflected in multiple files across the codebase, as indicated by the search results. No instances of the old import path were found, suggesting that the necessary updates have been made.store/snapshots/helpers_test.go (3)
238-242: The
Restore
method inhungCommitSnapshotter
is not implemented and panics. If this is intentional to indicate that the method should not be used, it's fine. However, if this method is expected to be implemented in the future, it would be helpful to add a TODO comment or create an issue to track this.270-290: The
SnapshotExtension
andRestoreExtension
methods inextSnapshotter
have been updated to use thesnapshots
package. Ensure that these changes are consistent with the intended use of thesnapshots
package and that all necessary functionality is present.17-25: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [291-308]
The
GetTempDir
function has been updated to useos.MkdirTemp
instead oftesting.T.TempDir
. This change is not mentioned in the summary. Verify that this change is intentional and does not affect the test behavior.store/snapshots/manager.go (8)
31-45: The restructuring of the
Manager
struct with the removal ofmultistore
and addition ofcommitSnapshotter
andstorageSnapshotter
aligns with the PR objectives to introduce a snapshot manager that works with the existing snapshot format. The changes to theextensions
map andlogger
field types are also noted and appear to be consistent with the rest of the codebase.77-90: The
NewManager
function has been correctly updated to reflect the changes in theManager
struct, ensuring that the new fieldscommitSnapshotter
andstorageSnapshotter
are initialized. The handling ofnil
extensions is also appropriate, providing a default empty map if none are provided.92-98: The
RegisterExtensions
method has been updated to use the newExtensionSnapshotter
type, which is consistent with the changes in theManager
struct. The error handling for duplicate snapshotter names and unsupported snapshot formats is also appropriate.166-174: The
Create
method has been updated to use thecommitSnapshotter
for snapshot creation, which is in line with the PR objectives to implement a snapshotter API for both commitment and storage stores. The error handling and defer statement to end the operation are correctly implemented.204-210: The
createSnapshot
method correctly delegates the snapshot creation to thecommitSnapshotter
, and the error handling withCloseWithError
is appropriate to ensure that any errors during the snapshot creation are propagated correctly.366-385: The
doRestoreSnapshot
method has been updated to use thestorageSnapshotter
for restoring the storage state, which aligns with the PR objectives. The channelchStorage
is used to pass the key-value pairs to thestorageSnapshotter
, and the error handling is consistent with the rest of the codebase.409-418: The
doRestoreSnapshot
method correctly handles the restoration of extensions and waits for thestorageSnapshotter
to complete. The error handling for the storage snapshotter is appropriate, ensuring that any errors are captured and returned.517-523: The
IsFormatSupported
function correctly checks if the snapshotter supports the given format, which is a necessary utility function for the snapshot manager to verify compatibility with different snapshot formats.store/snapshots/snapshotter.go (2)
10-24: The interfaces
CommitSnapshotter
andStorageSnapshotter
have been correctly defined to separate the concerns of commitment state and storage state snapshotting. This aligns with the PR objectives to introduce a snapshot manager that works with the existing snapshot format without breaking it.26-26: The
ExtensionSnapshotter
interface remains unchanged and is not part of the PR's objectives to modify snapshotting functionality.store/storage/database.go (2)
9-23: The
Database
interface is well-defined and aligns with the PR objectives to provide an abstraction layer for storage operations, including versioning and iteration capabilities. It is important to ensure that all implementations of this interface are consistent with the methods defined here.9-23: While the
Database
interface is well-defined, it is important to note that end-to-end tests for the new snapshot manager feature, including the implementations of this interface, are not included in this PR. It should be ensured that these tests are planned and tracked for future implementation to verify the integration with the base application.store/storage/pebbledb/db.go (3)
11-15: The addition of the
cosmossdk.io/store/v2/storage
import is consistent with the changes made to theDatabase
struct and the new snapshot manager functionality.29-32: The change from
store.VersionedDatabase
tostorage.Database
in the type assertion for theDatabase
struct aligns with the new snapshot manager's API and storage abstraction layer.96-102: The new
NewBatch
method added to theDatabase
struct is consistent with the snapshotter API implementation for storage stores. Ensure that theNewBatch
method is correctly integrated with the rest of the system and that its usage is covered by tests, especially since the PR notes that end-to-end tests are not included.
Verification result:
The
NewBatch
method has been successfully integrated into theDatabase
struct within thestore/storage/pebbledb/db.go
file, aligning with the snapshotter API for storage stores. The method's usage is confirmed in various parts of the system, includingstore/storage/store.go
,store/storage/rocksdb/db.go
,store/storage/sqlite/db.go
, andstore/storage/pebbledb/db.go
. However, it is important to ensure that this integration is accompanied by adequate unit tests, and the absence of end-to-end tests as noted in the PR should be addressed in subsequent updates.store/storage/pebbledb/db_test.go (1)
- 19-25: The changes in the
TestStorageTestSuite
function reflect the introduction of the new storage abstraction layer, as theNewDB
function now wraps thepebbledb
database withstorage.NewStorageStore
. This aligns with the PR's objective to implement a snapshotter API for storage stores. Ensure that theSetSync(false)
call and theEmptyBatchSize
configuration are appropriate for the test environment and consistent with the intended usage patterns.store/storage/rocksdb/db.go (4)
27-27: Ensure that the
Database
struct implements all methods required by thestorage.Database
interface to maintain compatibility.94-95: Confirm that the
NewBatch
function is defined and correctly creates a batch with the given version.91-100: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [12-98]
Verify that the removal of the
ApplyChangeset
method is intentional and assess the impact on other components that might use this method.
- 91-100: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [1-149]
The review is limited to the
store/storage/rocksdb/db.go
file and does not cover changes to theIavlTree
structure or theManager
struct mentioned in the summary.store/storage/rocksdb/db_test.go (3)
21-28: The update to the
NewDB
method in theTestStorageTestSuite
function correctly reflects the new signature and expected return type.34-45: The changes in the
TestDatabase_ReverseIterator
function, including the use ofNewBatch
andbatch.Write()
, are consistent with the new storage snapshotter API implementation.47-47: Ensure that the
ReverseIterator
method is correctly implemented and tested, as it is crucial for the snapshot restoration process.store/storage/sqlite/db.go (3)
11-15: The addition of the
"cosmossdk.io/store/v2/storage"
import aligns with the implementation of thestorage.Database
interface by theDatabase
struct.44-44: The
Database
struct correctly asserts that it implements thestorage.Database
interface, which is consistent with the PR's objective to introduce a snapshot manager.95-97: The addition of the
NewBatch
method to theDatabase
struct is a new feature that aligns with the snapshot manager's functionality.store/storage/sqlite/db_test.go (1)
- 19-26: : Please confirm that setting
EmptyBatchSize
to 0 inTestStorageTestSuite
is intentional and aligns with the expected behavior for the storage tests.store/storage/storage_bench_test.go (5)
4-4: The change in package declaration to
storage_test
ensures that the tests are black-box tests, which is a good practice in Go.27-40: The
backends
map now wraps the database instances withstorage.NewStorageStore
, aligning with the new snapshot manager feature.1-7: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [42-42]
The benchmarks correctly use the updated
backends
map to test the new storage abstraction.
22-23: The introduction of the
storeKey1
constant is likely for simulating a store key in the benchmarks, which aligns with the new snapshot manager feature.41-41: The use of
rng
for generating random keys and values in benchmarks is appropriate and common practice.store/storage/store.go (1)
- 20-130: The implementation of
StorageStore
aligns with the PR objectives to provide an abstraction layer for storage operations and implement the snapshotter API. The methods are consistent with the expected functionality of a storage abstraction in a snapshot manager system.
func (i *Importer) Close() error { | ||
i.importer.Close() | ||
|
||
return nil |
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.
The Close
method does not handle the error that might be returned by i.importer.Close()
. It is a best practice to handle errors when closing resources to ensure that any issues are properly logged or managed.
func (i *Importer) Close() error {
- i.importer.Close()
+ if err := i.importer.Close(); err != nil {
+ return err
+ }
return nil
}
Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
func (i *Importer) Close() error { | |
i.importer.Close() | |
return nil | |
func (i *Importer) Close() error { | |
if err := i.importer.Close(); err != nil { | |
return err | |
} | |
return nil | |
} |
// TODO: it is a random number, need to be tuned | ||
defaultBatchBufferSize = 100000 |
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.
Consider determining an optimal value for defaultBatchBufferSize
or providing a way to configure it dynamically based on the workload or environment.
i.importer.Close() | ||
|
||
return nil |
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.
i.importer.Close() | |
return nil | |
return i.importer.Close() |
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.
iavl importer returns nothing
// TODO: check the parallelism of this loop | ||
if err := func() error { |
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.
Why is this in a function closure?
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.
it is for defer expoerter.Close()
} | ||
}() | ||
|
||
nextItem, err = m.commitSnapshotter.Restore(snapshot.Height, snapshot.Format, streamReader, chStorage) |
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.
Question: why not restore SS and SC both async?
e.g.
eg := new(errgroup.Group)
eg.Go(func() error { /* restore SS */ })
eg.Go(func() error { /* restore SC */ })
if err := eg.Wait(); err != nil {
return fmt.Errorf("failed to restore: %w", err)
}
return nil
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.
SC restore is using the same routine of doRestoreSnapshot
, and it returns nextItem
which will be used in the extension restoring.
store/snapshots/manager.go
Outdated
|
||
snapshotMaxItemSize = int(64e6) // SDK has no key/value size limit, so we set an arbitrary limit | ||
) | ||
|
||
var ErrOptsZeroSnapshotInterval = errors.New("snaphot-interval must not be 0") | ||
|
||
// NewManager creates a new manager. | ||
func NewManager(store *Store, opts types.SnapshotOptions, multistore types.Snapshotter, extensions map[string]types.ExtensionSnapshotter, logger log.Logger) *Manager { | ||
func NewManager(store *Store, opts types.SnapshotOptions, commitSnapshotter CommitSnapshotter, storageSnapshotter StorageSnapshotter, extensions map[string]ExtensionSnapshotter, logger log.Logger) *Manager { |
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.
Shouldn't we move types/options.go
to the root as well? What is the point of types
package?
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.
it is a original snapshot structure
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.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (4)
- store/snapshots/manager.go (7 hunks)
- store/snapshots/manager_test.go (8 hunks)
- store/snapshots/options.go (1 hunks)
- store/storage/database.go (1 hunks)
Additional comments: 18
store/snapshots/manager.go (8)
31-43: The changes to the
Manager
struct, including the addition ofcommitSnapshotter
,storageSnapshotter
, andlogger
fields, align with the PR objectives to introduce a snapshot manager for the Cosmos SDK store system. These fields are essential for managing the snapshotting process for both the commitment and storage store as described.65-90: The updates to constants
chunkBufferSize
,chunkIDBufferSize
, anddefaultStorageChannelBufferSize
are consistent with the need to manage buffer sizes for snapshot operations. TheNewManager
function now correctly takescommitSnapshotter
andstorageSnapshotter
as parameters, which is in line with the PR's goal to implement a snapshotter API.166-174: The
Create
method's changes, including the removal of the call tomultistore.PruneSnapshotHeight
and the use ofcommitSnapshotter
, align with the PR's objectives to update the snapshot management logic.204-210: The
createSnapshot
method's changes, specifically the use ofcommitSnapshotter
to create a snapshot, are in line with the PR's objectives to implement a snapshotter API for the commitment store.366-385: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [366-419]
The
doRestoreSnapshot
method's changes, including the introduction of a channelchStorage
to pass KV pairs to the storage snapshotter and the error handling logic, are consistent with the PR's objectives to implement a snapshotter API for the storage store.
517-523: The
IsFormatSupported
function's changes, including the acceptance ofExtensionSnapshotter
instead oftypes.ExtensionSnapshotter
, align with the PR's objectives to update the snapshot interfaces.68-70: The previous comment regarding the introduction of
defaultStorageChannelBufferSize
as a global variable remains valid. The buffer size should be appropriate for the expected workload and not lead to excessive memory consumption.382-382: The conversation between
alexanderbez
andcool-develope
regarding the asynchronous restoration of SS and SC has been considered. The current implementation reflects a design decision to use the same routine for SC restore asdoRestoreSnapshot
, which returnsnextItem
for extension restoring.store/snapshots/manager_test.go (7)
16-16: The summary mentions the use of
types.NewSnapshotOptions
before the changes, but the provided code only showssnapshots.NewSnapshotOptions
. Please verify if this is an inconsistency or if the summary is incorrect.74-76: The summary does not mention the addition of
manager.RegisterExtensions(extSnapshotter)
calls in the test cases. This seems to be an important part of the test setup that should be included in the summary.24-25: The summary does not mention the use of
setupBusyManager(t)
to simulate a busy manager state in the tests. This seems to be an important part of the test logic that should be included in the summary.72-72: The summary does not mention the use of
newExtSnapshotter(10)
andnewExtSnapshotter(0)
in the test cases. This seems to be an important part of the test setup that should be included in the summary.95-95: The summary does not mention the use of
types.Metadata{ChunkHashes: checksums(chunks)}
in the test cases. This seems to be an important part of the test assertions that should be included in the summary.181-186: The summary does not mention the use of
manager.RestoreChunk(chunk)
in the test cases. This seems to be an important part of the test logic that should be included in the summary.239-239: The summary does not mention the use of
mockErrorCommitSnapshotter
in the test cases. This seems to be an important part of the test logic that should be included in the summary.store/snapshots/options.go (1)
- 1-4: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [3-14]
The
SnapshotOptions
struct andNewSnapshotOptions
function are correctly implemented and do not exhibit any issues.store/storage/database.go (2)
9-25: The
Database
interface provides a clear abstraction for storage operations, which aligns with the PR objectives to introduce a storage abstraction layer. The methods cover batch creation, key existence checks, value retrieval, version management, iteration, pruning, and closing operations, which are essential for the snapshot manager's functionality.9-25: Verify if the
Database
interface introduces duplication with the existingVersionedDatabase
in the rootstore
package, as mentioned by alexanderbez. If it does, consider refactoring to extend or useVersionedDatabase
to avoid code duplication.
Verification result:
The
Database
interface does not appear to introduce duplication with theVersionedDatabase
interface. The search results indicate thatVersionedDatabase
is used in various contexts throughout the codebase, and theDatabase
interface is a new addition that may serve a different purpose or provide a more specific abstraction for the storage system.
package snapshots | ||
|
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.
💡 NOTE
Codebase verification is a beta feature.
The package name change from types
to snapshots
in store/snapshots/options.go
has been confirmed. However, the search for the old package name types
indicates that numerous files across various modules still reference the old package name. These references need to be updated to reflect the new package name. The search for the new package name snapshots
confirms that several files within the store/snapshots
directory and one file in the api/cosmos/store/snapshots/v1
directory have been updated accordingly.
- Update all references from
package types
topackage snapshots
across the codebase as indicated by the search results. - Verify that the changes do not conflict with other modules that may also use a
types
package.
Original review comment
The package name has been changed to snapshots
. Ensure that all references to this package across the codebase are updated to reflect this change.
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.
left a few questions, otherwise looks good
return snapshotstypes.SnapshotItem{}, fmt.Errorf("received IAVL node item before store item") | ||
} | ||
node := item.IAVL | ||
if node.Height > int32(math.MaxInt8) { |
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.
why do we have this restriction, is it something related to iavl?
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.
yes, iavl requires int8
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.
hmm, we should change this in iavlv2, its weird versions can go up to maxint64 but iavl will crash before then. Seems like there is a bit of a misstep in expectations between the two dependencies
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.
I agree that in v2 of iavl we should remove this restriction. It makes code usage in iavl also awkward: one weekend many months ago before I found out about Matt's rewrite, I tried to tackle concurrency issues and data races in iavl using atomic.Int64 and it was awkward trying to use that in iavl.
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.
it is a just tree height, it can be up to maxUint8 (255)
@@ -175,29 +185,6 @@ func (db *Database) Get(storeKey string, targetVersion uint64, key []byte) ([]by | |||
return nil, nil | |||
} | |||
|
|||
func (db *Database) ApplyChangeset(version uint64, cs *store.Changeset) error { |
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.
could you explain why this was removed?
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.
it is moved to store to avoid the duplication ( the reason of abstraction layer in the storage)
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.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- store/root/store_test.go (2 hunks)
Additional comments: 3
store/root/store_test.go (3)
12-17: The addition of the
"cosmossdk.io/store/v2/storage"
import aligns with the PR's objective to introduce a new snapshot manager and related functionalities.33-41: The changes in the
SetupTest
method reflect the PR's objective to implement a storage abstraction layer by creating asqliteDB
and then wrapping it withstorage.NewStorageStore
.37-39: The
sc
variable now usesdefaultStoreKey
as a key in the map, which is a change from the previous version. This should be verified to ensure thatdefaultStoreKey
is correctly defined and used throughout the codebase.
The usage of
defaultStoreKey
is consistent across the codebase, with its definition found instore/root/store.go
and usage in various test and store files. No issues found with the introduction ofdefaultStoreKey
.
* feat: secp256k1 public key constant time (cosmos#18026) Signed-off-by: bizk <[email protected]> * chore: Fixed changelog duplicated items (cosmos#18628) * adr: Un-Ordered Transaction Inclusion (cosmos#18553) Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Aleksandr Bezobchuk <[email protected]> * docs: lint ADR-070 (cosmos#18634) * fix(baseapp)!: postHandler should run regardless of result (cosmos#18627) * docs: fix typos in adr-007-specialization-groups.md (cosmos#18635) * chore: alphabetize labels (cosmos#18640) * docs(x/circuit): add note on ante handler (cosmos#18637) Co-authored-by: Aleksandr Bezobchuk <[email protected]> * fix: telemetry metric label variable (cosmos#18643) * chore: typos fix (cosmos#18642) * refactor(store/v2): updates from integration (cosmos#18633) * build(deps): Bump actions/setup-go from 4 to 5 (cosmos#18647) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com> * feat(store/v2): snapshot manager (cosmos#18458) * chore(client/v2): fix typos in the README.md (cosmos#18657) * fix(baseapp): protocompat.go gogoproto.Merge does not work with custom types (cosmos#18654) Co-authored-by: unknown unknown <unknown@unknown> * chore: fix several minor typos (cosmos#18660) * chore(tools/confix/cmd): fix typo in view.go (cosmos#18659) * refactor(x/staking): check duplicate addresses in StakeAuthorization's params (cosmos#18655) * feat(accounts): use gogoproto API instead of protov2. (cosmos#18653) Co-authored-by: unknown unknown <unknown@unknown> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * fix(store/commitment/iavl): honor tree.Remove error firstly (cosmos#18651) * build(deps): Bump actions/stale from 8 to 9 (cosmos#18656) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * chore(docs): fix typos & wording in docs (cosmos#18667) * chore: fix several typos. (cosmos#18666) * feat(telemetry): enable `statsd` and `dogstatsd` telemetry sinks (cosmos#18646) Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Aleksandr Bezobchuk <[email protected]> Co-authored-by: marbar3778 <[email protected]> Co-authored-by: Marko <[email protected]> * feat(store/v2): add SetInitialVersion in SC (cosmos#18665) * feat(client/keys): support display discreetly for `keys add` (cosmos#18663) Co-authored-by: Julien Robert <[email protected]> * ci: add misspell action (cosmos#18671) * chore: typos fix by misspell-fixer (cosmos#18683) Co-authored-by: github-merge-queue <[email protected]> Co-authored-by: Julien Robert <[email protected]> * chore: add v0.50.2 changelog to main (cosmos#18682) * build(deps): Bump github.com/jhump/protoreflect from 1.15.3 to 1.15.4 in /tests (cosmos#18678) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * refactor(bank): remove .String() calls (cosmos#18175) Co-authored-by: Facundo <[email protected]> * ci: use codespell instead of misspell-fixer (cosmos#18686) Co-authored-by: Marko <[email protected]> * feat(gov): add proposal types and spam votes (cosmos#18532) * feat(accounts): use account number as state prefix for account state (cosmos#18664) Co-authored-by: unknown unknown <unknown@unknown> * chore: typos fixes by cosmos-sdk bot (cosmos#18689) Co-authored-by: github-merge-queue <[email protected]> Co-authored-by: Julien Robert <[email protected]> Co-authored-by: marbar3778 <[email protected]> * feat(client/keys): support display discreetly for keys mnemonic (cosmos#18688) * refactor: remove panic usage in keeper methods (cosmos#18636) * ci: rename pr name in misspell job (cosmos#18693) Co-authored-by: Marko <[email protected]> * build(deps): Bump github.com/pelletier/go-toml/v2 from 2.1.0 to 2.1.1 in /tools/confix (cosmos#18702) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com> * feat(client/keys): support display discreetly for keys export (cosmos#18684) * feat(x/gov): better gov genesis validation (cosmos#18707) --------- Signed-off-by: bizk <[email protected]> Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: Carlos Santiago Yanzon <[email protected]> Co-authored-by: yihuang <[email protected]> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Aleksandr Bezobchuk <[email protected]> Co-authored-by: Facundo Medica <[email protected]> Co-authored-by: Akaonetwo <[email protected]> Co-authored-by: Marko <[email protected]> Co-authored-by: Julien Robert <[email protected]> Co-authored-by: dreamweaverxyz <[email protected]> Co-authored-by: Pioua <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: cool-developer <[email protected]> Co-authored-by: leonarddt05 <[email protected]> Co-authored-by: testinginprod <[email protected]> Co-authored-by: unknown unknown <unknown@unknown> Co-authored-by: Sukey <[email protected]> Co-authored-by: axie <[email protected]> Co-authored-by: Luke Ma <[email protected]> Co-authored-by: Emmanuel T Odeke <[email protected]> Co-authored-by: 0xn4de <[email protected]> Co-authored-by: hattizai <[email protected]> Co-authored-by: Devon Bear <[email protected]> Co-authored-by: Marko <[email protected]> Co-authored-by: Halimao <[email protected]> Co-authored-by: Cosmos SDK <[email protected]> Co-authored-by: github-merge-queue <[email protected]> Co-authored-by: Facundo <[email protected]> Co-authored-by: Likhita Polavarapu <[email protected]>
I'm trying to restore chain's data from the snapshot manager using @yihuang snapshot cli. Here is the reference: #16067 I have posted the error i'm receiving in that PR. Did this recent PR break the snapshot cli? When trying to do a snapshot statesync, I received the following error when starting the chain:
|
Description
ref: #18329
NOTE:
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!
to the type prefix if API or client breaking changeCHANGELOG.md
make lint
andmake test
Reviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
!
in the type prefix if API or client breaking changeSummary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor
StorageStore
abstraction.Manager
struct to accommodate new snapshotter interfaces.Style
Tests
Chores