Skip to content
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

Datastore copy util #469

Merged
merged 6 commits into from
Dec 9, 2020
Merged

Datastore copy util #469

merged 6 commits into from
Dec 9, 2020

Conversation

sanderpick
Copy link
Member

Adds a small program for copying a badger/mongo datastore to another badger/mongo datastore.

@sanderpick sanderpick self-assigned this Dec 8, 2020
Signed-off-by: Sander Pick <sanderpick@gmail.com>
@sanderpick sanderpick force-pushed the sander/datastore-migration branch from 75147de to bdd8cab Compare December 8, 2020 08:24
Copy link
Contributor

@jsign jsign left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! I left two somewhat important comments to consider.

Comment on lines 117 to 118
for r := range results.Next() {
if err := to.Put(ds.NewKey(r.Key), r.Value); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Between these two lines we need to add:

if r.Error != nil {
   log.Fatal(...)
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yeah, thanks!

for r := range results.Next() {
if err := to.Put(ds.NewKey(r.Key), r.Value); err != nil {
log.Fatalf("copying %s: %v", r.Key, err)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I discovered while working and testing Powergate migrations that this is too slow for copying (at least for my taste).

Here is what I did for migrations; which reduced times by some orders of magnitude. Nothing crazy, ignoring the migration logic, basically some rate-limiting to Read-and-Put up to 1000 in concurrently instead of serially.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, copied your approach there

Signed-off-by: Sander Pick <sanderpick@gmail.com>
Signed-off-by: Sander Pick <sanderpick@gmail.com>
Signed-off-by: Sander Pick <sanderpick@gmail.com>
Signed-off-by: Sander Pick <sanderpick@gmail.com>
@sanderpick
Copy link
Member Author

Sorry the big code update post review here, but I realized using this tool would be much easier if it wasn't a generic datastore copier, but was specific to the structure of go-threads's datastores.

},
{
name: "ipfslite",
files: []string{"key"},
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move the key as well...

Copy link
Contributor

@jsign jsign left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Left a comment.

lock.Lock()
count++
lock.Unlock()
if count%parallel == 0 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: move lock.Unlock() to include the if case since count is being read without lock and might be a data-race with other goroutine owning the lock. (tongue twister)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, done

Signed-off-by: Sander Pick <sanderpick@gmail.com>
@sanderpick sanderpick merged commit 4b2596f into master Dec 9, 2020
@sanderpick sanderpick deleted the sander/datastore-migration branch December 9, 2020 21:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants