-
-
Notifications
You must be signed in to change notification settings - Fork 148
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
Add CLI for migration #1035
Add CLI for migration #1035
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.
Thanks for your contribution. Overall looks good. I have a few questions.
A. migrations
directory
Currently, Yorkie is built and distributed as a single binary(yorkie
). In this change, it seems that Yorkie CLI is designed to execute by referencing the migrations
directory.
While migrations
directory is included in the binary during the build, it is unclear why the command(yorkie migration
) still needs to reference the directory.
It would be better to retrieve and execute the target scripts based on the migrationMap
rather than external directory. I'm curious about your opinion.
B. Batch Processing for Find
Please, consider implementing batch processing for Find
. The current approach of loading the entire collection into memory could lead to excessive memory usage.
collection := db.Database(databaseName).Collection("changes")
cursor, err := collection.Find(ctx, bson.M{})
if err != nil {
return err
}
1bbc11d
to
54cc830
Compare
A. Agree, you're right. Watching migration map is enough. I'll fix it. |
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.
Thanks for your contribution.
What this PR does / why we need it:
Define semi-auto migration architecture and add migration scripts for add version vector.
Process
Usage example
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
Additional documentation:
Checklist: