-
Notifications
You must be signed in to change notification settings - Fork 5k
feat: [6654385780] show snap progress #34203
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
base: 3.0
Are you sure you want to change the base?
Conversation
Summary of ChangesHello @cadem, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces significant enhancements to the synchronization mechanisms by adding detailed tracking for snapshot sequences and total index. These new metrics provide a clearer picture of the replication state, especially for learner nodes and during snapshot operations. The changes involve extending several key data structures, updating message handling for these new fields, and refining the logging and display of synchronization progress across different components. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request introduces new fields snapSeq and syncTotalIndex to various synchronization-related data structures (SVnodeLoad, SSyncState, SVnodeGid, SSyncNode) to enhance snapshot and synchronization state tracking. The changes include updating serialization and deserialization logic for these fields, initializing them in relevant contexts, and modifying logging and state update functions to incorporate them. The TSDB_SYNC_APPLY_COMMIT_LEN macro was increased, and the syncNodeStartSnapshot function now accepts a reason parameter for improved logging. Additionally, the mndRetrieveVgroups function's output formatting for sync status was updated to display these new fields. Review comments suggest adding the copying of syncTotalIndex in mndVgroupActionUpdate for consistency, moving the SYNC_SNAPSHOT_SEQ_END macro to a common header (tdef.h) to avoid duplication and use INT32_MAX, and refactoring the applyStr formatting logic in mndRetrieveVgroups for better readability and to eliminate redundant logging.
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.
Pull request overview
This PR adds snapshot sequence (snapSeq) and total index (totalIndex) tracking to sync structures, enhancing observability of snapshot replication and learner progress in the synchronization subsystem.
Key Changes:
- Added
snapSeqandtotalIndexfields to sync state structures (SSyncState,SVnodeLoad,SVnodeGid) to track snapshot replication progress - Updated
syncNodeStartSnapshotfunction signature to include areasonparameter for better logging and diagnostics - Enhanced display output in vgroup queries to show snapshot sequence and learner progress metrics
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| source/libs/sync/inc/syncInt.h | Added snapSeq field to SSyncNode structure and updated syncNodeStartSnapshot function signature |
| source/libs/sync/src/syncSnapshot.c | Updated syncNodeStartSnapshot to accept reason parameter and set snapSeq throughout snapshot lifecycle |
| source/libs/sync/src/syncPipeline.c | Updated calls to syncNodeStartSnapshot with reason strings and initialized snapSeq before triggering snapshots |
| source/libs/sync/src/syncMain.c | Initialized snapSeq to -1 in syncNodeOpen and populated state fields in syncGetState |
| include/libs/sync/sync.h | Added snapSeq and totalIndex fields to SSyncState structure |
| source/dnode/vnode/src/vnd/vnodeQuery.c | Populated new snapSeq and syncTotalIndex fields in vnodeGetLoad |
| source/dnode/mnode/impl/inc/mndDef.h | Added snapSeq and syncTotalIndex fields to SVnodeGid structure |
| source/dnode/mnode/impl/src/mndVgroup.c | Enhanced display logic to show snapshot and learner progress with new format; defined SYNC_SNAPSHOT_SEQ_END constant |
| source/dnode/mnode/impl/src/mndMain.c | Reset learnerProgress and snapSeq when setting vgroup offline |
| source/dnode/mnode/impl/src/mndDnode.c | Updated vnode state with new fields and added logging for learner/snapshot progress; defined SYNC_SNAPSHOT_SEQ_END constant |
| include/common/tmsg.h | Added snapSeq and syncTotalIndex fields to SVnodeLoad structure |
| source/common/src/msg/tmsg.c | Added serialization/deserialization for snapSeq and syncTotalIndex fields in status request messages |
| include/util/tdef.h | Increased TSDB_SYNC_APPLY_COMMIT_LEN from 41 to 100 to accommodate enhanced display format |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ructures
Description
Issue(s)
Checklist
Please check the items in the checklist if applicable.