-
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
docs(store/v2): Add missing documents #22118
Conversation
📝 WalkthroughWalkthroughThe pull request introduces significant updates to the documentation across several packages within the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
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.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (9)
store/v2/commitment/README.md (1)
31-35
: Excellent addition of the Pruning section.The new section on Pruning is well-written and provides valuable information about the process and implementation requirements. It aligns perfectly with the AI-generated summary and enhances the overall documentation.
Consider adding a brief explanation of what the
PruningOption
is, as it's mentioned but not defined. This would provide more context for readers unfamiliar with the concept.store/v2/README.md (4)
45-47
: LGTM: Improved clarity on pruning processThe correction of the method name to
PruneStoreKeys
and the added explanation about the pruning process enhance the documentation's accuracy and clarity.Consider adding a brief note about when the actual data removal occurs, if that information is available. For example:
"The actual data removal is done by the pruning process of the underlying SS and SC, which typically occurs [insert timing or trigger for pruning]."
51-52
: LGTM: Added Migration sectionThe new Migration section provides valuable information about the migration process from store/v1 to store/v2, which is crucial for users upgrading their systems.
Consider expanding this section slightly to include:
- A brief mention of why someone might need to migrate (e.g., "For users upgrading from store/v1...")
- Any high-level steps or considerations for the migration process.
This additional context could help users better understand the migration process without needing to immediately refer to the detailed documentation.
61-64
: LGTM: Added State Sync sectionThe new State Sync section clearly states the responsibility boundary of root.Store and directs users to the appropriate documentation for more information.
To provide more context, consider expanding this section slightly:
- Add a brief explanation of what state sync is and why it's important.
- Mention which component is responsible for state sync if root.Store is not.
For example:
"State sync allows new nodes to quickly catch up to the current state of the blockchain. While the root.Store is NOT responsible for state sync, this crucial functionality is handled by [component name]. See Snapshots Manager for more details on implementation and usage."
57-57
: Minor: Consider hyphenation of "asynchronous"The static analysis tool suggests hyphenating "asynchronous" as "asynchron-ous". However, in modern usage, "asynchronous" is commonly written without a hyphen and is correct as is. You may choose to keep it as-is or add the hyphen based on your project's style guide.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~57-~57: This expression is ususally spelled with a hyphen
Context: ...nd SC layers. This means pruning can be implementation specific, such as being synchronous or asynchron...(SPECIFIC_HYPHEN)
store/v2/migration/README.md (2)
24-49
: LGTM: Clear usage instructions with a minor suggestionThe usage section provides clear instructions on how to create and use the migration manager. The code snippets are well-formatted and informative.
Consider adding a brief example of how to call the
Start
method, including how to create and pass the context. This would provide a complete usage example for developers.
111-111
: Add a newline character at the end of the fileTo comply with Markdown best practices and resolve the static analysis hint, please add a newline character at the end of the file.
Apply this change to the end of the file:
Alternatively, keeping store/v1 accessible for historical queries could be an option. +
🧰 Tools
🪛 Markdownlint
111-111: null
Files should end with a single newline character(MD047, single-trailing-newline)
store/v2/storage/README.md (2)
73-76
: Excellent addition to the Pruning section!The new content provides a clear and concise explanation of the pruning process and its implementation in the State Storage system. It introduces important concepts like the
Pruner
interface andPruningManager
, which enhance the reader's understanding of the pruning mechanism.To further improve clarity, consider adding a brief example or more details about the
PruningOption
mentioned in the last line. This could help readers better understand how pruning can be configured.
Line range hint
78-87
: Great improvements to the State Sync section!The modifications provide a clearer and more detailed explanation of the state sync process, particularly regarding the roles of
snapshots.Manager
andStorageStore
. The expanded description of theRestore
method is especially helpful in understanding its functionality.To make this section even more comprehensive, consider adding a brief mention of how the
snapshots.Manager
creates snapshots, not just how it restores them. This would provide a complete picture of the state sync process.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (4)
- store/v2/README.md (2 hunks)
- store/v2/commitment/README.md (1 hunks)
- store/v2/migration/README.md (1 hunks)
- store/v2/storage/README.md (1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
store/v2/README.md (1)
Pattern
**/*.md
: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"store/v2/commitment/README.md (1)
Pattern
**/*.md
: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"store/v2/migration/README.md (1)
Pattern
**/*.md
: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"store/v2/storage/README.md (1)
Pattern
**/*.md
: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
🪛 LanguageTool
store/v2/README.md
[uncategorized] ~57-~57: This expression is ususally spelled with a hyphen
Context: ...nd SC layers. This means pruning can be implementation specific, such as being synchronous or asynchron...(SPECIFIC_HYPHEN)
🪛 Markdownlint
store/v2/migration/README.md
111-111: null
Files should end with a single newline character(MD047, single-trailing-newline)
🔇 Additional comments (7)
store/v2/README.md (2)
4-4
: LGTM: Corrected link to ADR-065The update to the relative path for the ADR-065 document improves the accuracy of the documentation.
58-58
: LGTM: Added link to Pruning Manager documentationThe addition of the link to the Pruning Manager documentation provides a valuable resource for users seeking more detailed information about the pruning process.
store/v2/migration/README.md (4)
1-23
: LGTM: Clear and informative introductionThe introduction and overview sections provide a concise and clear explanation of the migration manager's purpose and the migration process. The note about historical queries not being supported after migration is crucial information for users.
50-79
: LGTM: Well-structured migration flow diagramThe migration flow section provides an excellent visual representation of the process using a mermaid sequence diagram. The diagram clearly illustrates the interactions between different components involved in the migration, making it easy for developers to understand the flow.
80-111
: LGTM: Comprehensive key considerationsThe key considerations section provides valuable information about the migration process, including its lazy nature, background operation, handling of failures, and impact on historical queries. This information is crucial for users to understand the implications of the migration process.
🧰 Tools
🪛 Markdownlint
111-111: null
Files should end with a single newline character(MD047, single-trailing-newline)
1-111
: LGTM: Well-structured and formatted documentThe overall structure and formatting of the document are excellent. The use of clear headings, properly formatted code snippets, and the mermaid diagram enhances readability and makes the information easy to understand.
🧰 Tools
🪛 Markdownlint
111-111: null
Files should end with a single newline character(MD047, single-trailing-newline)
store/v2/storage/README.md (1)
Line range hint
1-100
: Overall excellent improvements to the documentation!The changes made to this README file have significantly enhanced the clarity and completeness of the State Storage (SS) documentation. The additions to the Pruning and State Sync sections provide valuable insights into these processes, making the document more informative for users and developers.
The new content integrates well with the existing structure, maintaining consistency throughout the document. These improvements will undoubtedly help readers better understand the functionality and implementation of the State Storage system in the Cosmos SDK.
1. **Create a snapshot** of the current state while `Commit` operations continue to | ||
function with `store/v1`. | ||
2. **Restore the snapshot** into the new StateStorage (SS) and StateCommitment (SC). | ||
3. **Sync recent state changes** from `store/v1` to the new SS and SC. | ||
4. After syncing, the `Commit` operation will be switched to the new `store/v2`. | ||
|
||
Taking a snapshot is a lightweight operation. The snapshot is not stored on disk but | ||
consumed by the `Restore` process, which replays state changes to the new SS and SC. | ||
|
||
> **Note:** After migration, `store/v2` does **not** support historical queries. | ||
If historical data access is required, a full state migration to `store/v2` is necessary. | ||
|
||
## Usage | ||
|
||
You can create a new `migration.Manager` by calling the following function: | ||
|
||
```go | ||
func NewManager( | ||
db corestore.KVStoreWithBatch, | ||
sm *snapshots.Manager, | ||
ss *storage.StorageStore, | ||
sc *commitment.CommitStore, | ||
logger log.Logger | ||
) *Manager | ||
``` | ||
|
||
* `sc` (Commitment Store) can be `nil`. In that case, the Manager will migrate only | ||
the state storage. | ||
* The migration process is lazy, meaning data is migrated in the background while | ||
`root.Store` remains fully operational. | ||
|
||
To initiate the migration process, call the `Start` method: | ||
|
||
```go | ||
func (m *Manager) Start(ctx context.Context) error | ||
``` | ||
|
||
> **Note:** It should be called by the RootStore, running in the background. | ||
|
||
## Migration Flow | ||
|
||
```mermaid | ||
sequenceDiagram | ||
autonumber | ||
|
||
participant A as RootStore | ||
participant B as MigrationManager | ||
participant C as SnapshotsManager | ||
participant D as StateCommitment | ||
participant E as StateStorage | ||
|
||
A->>B: Start | ||
loop Old Data Migration | ||
B->>C: Create Snapshot | ||
C->>B: Stream Snapshot | ||
B->>D: State Sync (Restore) | ||
B->>E: Write Changeset (Restore) | ||
end | ||
|
||
loop New Commit Data Sync | ||
A->>B: Commit(Changeset) | ||
B->>B: Store Changeset | ||
B->>D: Commit Changeset | ||
B->>E: Write Changeset | ||
end | ||
|
||
B->>A: Switch to new store/v2 | ||
``` | ||
|
||
## Key Considerations | ||
|
||
### Laziness and Background Operation | ||
|
||
The migration is performed lazily, meaning it occurs in the background without | ||
interrupting the current operations on root.Store. This allows the chain to continue | ||
running while data is gradually migrated to `store/v2`. State synchronization ensures | ||
that any new state changes during the migration are also applied to `store/v2`. | ||
|
||
However, note that there may be a performance impact depending on the size of the data | ||
being migrated, and it’s essential to monitor the migration process in production | ||
environments. | ||
|
||
### Handling Failures and Rollbacks | ||
|
||
It is important to consider how the migration manager handles errors or system failures | ||
during the migration process: | ||
|
||
* If the migration fails, there is no impact on the existing `store/v1` operations, | ||
but need to restart the migration process from the scratch. | ||
* In the event of a critical failure after migration, a rollback may not be possible, | ||
and it is needed to keep the `store/v1` backup for a certain period. | ||
|
||
### Impact on Historical Queries | ||
|
||
After the migration, the new `store/v2` does not support historical queries. | ||
This limitation should be clearly understood before starting the migration process, | ||
especially if the node relies on historical data for any operations. | ||
|
||
If historical queries are required, users must fully migrate all historical data to `store/v2`. | ||
Alternatively, keeping store/v1 accessible for historical queries could be an option. |
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.
🛠️ Refactor suggestion
Consider adding sections on testing and troubleshooting
The document provides comprehensive information about the migration process. To further enhance its usefulness, consider adding the following sections:
- Testing and Validation: Briefly describe any recommended steps for testing or validating the migration process after completion.
- Troubleshooting: Include a section on common issues that might arise during migration and how to resolve them.
These additions would provide a more complete guide for users undertaking the migration process.
🧰 Tools
🪛 Markdownlint
111-111: null
Files should end with a single newline character(MD047, single-trailing-newline)
Description
Closes: #XXXX
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...
!
in the type prefix if API or client breaking changeCHANGELOG.md
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.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
root.Store
type and its functionalities in thestore/v2
documentation.store/v1
tostore/v2
.commitment
package documentation, detailing data management processes.storage
package documentation to clarify the roles of various components and pruning processes.