Add migration script to detach documents from deactivated clients #1062
Conversation
WalkthroughThe changes introduce a new migration version (v0.5.6) to the migration functionality in the Yorkie application. This includes updates to the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used📓 Learnings (1)migrations/v0.5.6/document-detach.go (1)🔇 Additional comments (2)migrations/v0.5.6/document-detach.go (2)
The migration script is well-structured and aligns with the PR objectives. Error handling and logging are appropriately implemented.
Correct the progress percentage calculation to reflect actual progress Currently, the progress percentage is calculated using Apply this diff to use - percentage := int(float64(batchSize*batchCount) / float64(totalCount) * 100)
+ percentage := int(float64(done) / float64(totalCount) * 100)⛔ Skipped due to learningsThank 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
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1062 +/- ##
=======================================
Coverage 46.82% 46.82%
=======================================
Files 84 84
Lines 12180 12180
=======================================
Hits 5703 5703
Misses 5909 5909
Partials 568 568 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (2)
migrations/v0.5.6/main.go (1)
17-18: Enhance package documentation with migration purposeThe package documentation should provide more context about the specific purpose of this migration. Consider adding details about reactivating clients with attached documents to enable proper document detachment.
-// Package v056 provides migration for v0.5.6 +// Package v056 provides migration for v0.5.6 to handle document detachment from +// deactivated clients. It temporarily reactivates clients that have attached +// documents to allow the housekeeping process to properly detach these documents. package v056cmd/yorkie/migration.go (1)
46-48: Consider migration safety and rollback strategy.Given that this migration modifies client activation states which affects document attachments:
- Ensure the migration is idempotent and can be safely re-run
- Consider adding a dry-run mode to preview the changes
- Document the rollback procedure in case of partial migration failure
Would you like me to help draft the documentation for these safety considerations?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
cmd/yorkie/migration.go(2 hunks)migrations/v0.5.6/document-detach.go(1 hunks)migrations/v0.5.6/main.go(1 hunks)
🔇 Additional comments (5)
migrations/v0.5.6/main.go (3)
20-24: LGTM!
The imports are clean and contain only the necessary packages.
1-34: Verify migration system integration
Let's ensure this migration is properly integrated into the migration system and follows the correct version sequence.
#!/bin/bash
# Check migration registration and version sequence
echo "Checking migration registration..."
rg -A 5 'migrationMap\[' cmd/yorkie/migration.go
echo "Checking for other v0.5.x migrations..."
fd -t d '^v0\.5\.' migrations/26-34: 🛠️ Refactor suggestion
Improve function documentation and error handling
The function implementation needs several improvements:
- The function documentation should describe the purpose and parameters
- The error handling could be more specific about what went wrong
- The batchSize parameter's valid range should be validated
-// RunMigration runs migrations for v0.5.6
+// RunMigration executes the v0.5.6 migration to reactivate clients that have
+// attached documents, allowing the housekeeping process to properly detach them.
+// The batchSize parameter controls how many clients are processed in each batch.
+//
+// Parameters:
+// - ctx: Context for the operation
+// - db: MongoDB client
+// - databaseName: Name of the database
+// - batchSize: Number of clients to process in each batch (must be > 0)
func RunMigration(ctx context.Context, db *mongo.Client, databaseName string, batchSize int) error {
+ if batchSize <= 0 {
+ return fmt.Errorf("invalid batch size: %d, must be greater than 0", batchSize)
+ }
+
err := ReactivateClients(ctx, db, databaseName, batchSize)
if err != nil {
- return err
+ return fmt.Errorf("failed to reactivate clients: %w", err)
}
return nil
}Let's verify the ReactivateClients implementation:
cmd/yorkie/migration.go (2)
33-33: Verify the v0.5.6 migration package implementation.
The import looks correct. Let's verify the implementation of the migration package.
#!/bin/bash
# Description: Verify the v0.5.6 migration package implementation
# Check if the migration package exists and contains the required files
fd -t f "main.go|document-detach.go" -p "migrations/v0.5.6"
# Verify RunMigration function signature in the v0.5.6 package
ast-grep --pattern 'func RunMigration(ctx context.Context, db *mongo.Client, dbName string, batchSize int) error'47-47: Verify version ordering in migrationMap.
The new version entry follows the correct pattern. However, let's ensure there are no missing versions between v0.5.3 and v0.5.6 that should be included.
What this PR does / why we need it?
This PR introduces a migration script to properly detach documents that are still attached to deactivated clients.
While PRs #907 and #1036 improved the housekeeping process to ensure documents can be detached correctly, there may still be documents attached to deactivated clients from before these changes were applied.
Given the complexity of handling presence clears and version vector deletions during the detachment process, this migration script does not directly detach the documents. Instead, it updates the relevant clients from deactivated to activated, allowing the housekeeping process to manage the actual detachment of documents as intended.
Any background context you want to provide?
Deactivated clients may still have attached documents, which can lead to complications in managing document states. By running this migration script, we ensure a cleaner and more reliable detachment process for documents linked to clients that are no longer active.
What are the relevant tickets?
Related #602
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
Additional documentation:
Checklist:
Summary by CodeRabbit