Fixes applied to migration process

[emmanuelm41]

It solves issues reported on #855

[hsanjuan]

It's probably best practice (and likely faster), to wrap the reader/writers with bufio. Also, perhaps it would be more solid to do os.Rename and then copy back the file into the original location, and if that fails, perhaps rename the files back.

In one case, the node can go on without the backup, even on error when creating it. On the other, it cannot go on. One likely scenario might be a node with little disk space left, that cannot duplicate the DB. It will fail again and again to upgrade with the current approach. When renaming, it will upgrade just fine and could potentially continue.

[nikkolasg]

I'm personally fine for this change - the DB should not be THAT big, the problem is more out of memory error, which is a recurrent drand issue about go routine leakage I think.
So using io.Copy with streams is good for me.

[hsanjuan]

So using io.Copy with streams is good for me.

So with bare io.Copy we cannot control how much is read and written. Ideally we will be writing in large enough chunks that our copy does not take longer than needed (this can be actually benchmarked). bufio potenitally solves that with a large enough buffer. (This assumption can be tested).

I think the patch potentially solves the issue, but if we can level-up the quality as much as possible while doing that, I don't see the problem in the extra 2 lines of code.

[nikkolasg]

What do you think about just using the "cp" cmd actually instead of re implementing cp again ? Letting the OS do it is probably the easiest and fastest way no ?

…

On Wed, 1 Dec 2021, 13:28 Hector Sanjuan, ***@***.***> wrote: So using io.Copy with streams is good for me. So with bare io.Copy we cannot control how much is read and written. Ideally we will be writing in large enough chunks that our copy does not take longer than needed (this can be actually benchmarked). bufio potenitally solves that with a large enough buffer. (This assumption can be tested). I think the patch potentially solves the issue, but if we can level-up the quality as much as possible while doing that, I don't see the problem in the extra 2 lines of code. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#862 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AATSFC2BSM7XSNWKTKJN3J3UOYIITANCNFSM5JCQKWCQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

[emmanuelm41]

It's probably best practice (and likely faster), to wrap the reader/writers with bufio. Also, perhaps it would be more solid to do os.Rename and then copy back the file into the original location, and if that fails, perhaps rename the files back.

In one case, the node can go on without the backup, even on error when creating it. On the other, it cannot go on. One likely scenario might be a node with little disk space left, that cannot duplicate the DB. It will fail again and again to upgrade with the current approach. When renaming, it will upgrade just fine and could potentially continue.

The original files are left untouched as the process must allow to execute a rollback if something goes wrong. That is something the team wanted. I try not to touch the original files so the risk is minimum. I personally think that is a good approach in this case.

Talking about bufio, I can add it to the migration process.

What do you think about just using the "cp" cmd actually instead of re
implementing cp again ? Letting the OS do it is probably the easiest and
fastest way no ?

I am not sure if the control we have over the copy process is better or not. I feel confortable to copy files from golang

[hsanjuan]

What do you think about just using the "cp" cmd actually instead of re
implementing cp again ? Letting the OS do it is probably the easiest and
fastest way no ?

No, we don't call the OS for these things. Nothing tells you that there is a shell to invoke, a cp command present in the system etc. In general invoking shell commands is going to be a bad thing in 99% of cases, if there is an alternative.

[emmanuelm41]

@hsanjuan could you please approve it if you see it is correct now? 😄 Thanks!

[nikkolasg]

Fine if we want to use buffered io - in this use case I dont see it as a requirement since we are reading from a local file so we already have all the data to fill the buffer, not like TCP socket etc- But I am not fine to re-implement things that golang already provides. The copy method from Go is much more fine grained tested and we should use that.
If we want to control the size of the buffer (which is the only detail that differ between using io.Copy and buff writer from what I see), can we use native features such as https://pkg.go.dev/io#CopyBuffer ? It actually uses the same logic as io.Copy but with the buffer we want which is exactly what @emmanuelm41 has implemented here.

[emmanuelm41]

@nikkolasg I just applied the change you suggested. What do you think now?