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

Use zfs recv -u instead of setting destination properties one by one #38

Closed
foxycode opened this issue Feb 27, 2017 · 9 comments
Closed
Assignees

Comments

@foxycode
Copy link

When I played with script, I noticed that destination properties are set one by one when recv -x is not supported. I also saw from code comments that you like to always send with -p option. There is possibility to send with -p option and receive with -u option so destination filesystem isn't mounted and mountpoint can be fixed after transfer.

In my pull #36 (comment) nothing changed against your original approach, destination is mounted at the end, no init-unmounted is needed. My code could probably be used instead of else section and settings properties one by one, but I wasn't sure, I know only SMartOS so I left it intact.

@ppbrown
Copy link
Member

ppbrown commented Feb 27, 2017

So.. I'm missing something.

If the end result is the same anyway.. then what is the benefit of using -u ?
:-}

Is the benefit "dont have to do the manual setting of properties"?
So, is your suggestion purely a code cleanup suggestion, and doesnt have any end user benefit whatsoever? :-D

Although, given that -x is not widely supported, sadly.. perhaps you could say that "faster initialization for more OS's" is the end user benefit?

@foxycode
Copy link
Author

Problem is, more commands you send over network, more can broke up. With this fix you can send with all properties right away and won't be surprised they are missing when you need to rollback. But yes, it's sort of code cleanup and yes, it's little faster than setting properties one by one.

@ppbrown ppbrown self-assigned this Feb 27, 2017
@ppbrown
Copy link
Member

ppbrown commented Feb 27, 2017

Okay, understood at last.
This is now officially a "new feature when I get to it" issue now :)

Given that initialization is especially crucial to zrep user experience, I'm going to need to spend some quality time thinking about my preferred coding approach, analyzing and testing edge cases,etc.

@foxycode
Copy link
Author

Aaaaaan this is why I reather send PR's :) Can I rework my PR somehow so you will merge it?

@ppbrown
Copy link
Member

ppbrown commented Feb 27, 2017

Thanks for the enthusiasm, but.... no :)
I want to do it myself.
zrep is 100% my own code and I intend to keep it that way. For multiple reasons.
1 is that it just simplifies any potential copyright issues. This is not GPL code. This is MY code, and I intend to keep it that way.
2 is that I would probably spend more time going back and forth with you to tweak your code to the point where I am happy with it, than writing it myself ;) I am hyper-picky about code.
For what it's worth, I seem to recall that you actually did a fairly good job of copying my style.. but....
no :)

@foxycode
Copy link
Author

It's strange for me that you have code on github and don't like anyone to contribute. Me on the other hand like opensource very much and trying to contribute where I can.

I wanted to make your great script better for everyone reather than writing my own solution. I see it as a win-win. I'm also sorry that you are thinking like this about licensing, but it's your right.

I will write my own script after all.

@ppbrown
Copy link
Member

ppbrown commented Feb 27, 2017

Thanks for pointing out the -u option. I'll probably code something today or tomorrow, fyi.

@ppbrown
Copy link
Member

ppbrown commented Mar 1, 2017

I updated the code to use recv -u instead of recv -x
I also LEFT it unmounted.
(but updated output to let people know that is what is happening)
Works for me.
If you feel like testing it, lemme know how it goes on gentoo, and I'll release it.

@foxycode
Copy link
Author

foxycode commented Mar 1, 2017

Where I was talking about gentoo? :) I am using SmartOS. Will test on weekend, thanks.

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

No branches or pull requests

2 participants