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

WIP cmd_restore: expose filerestorer options in flags #2178

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
3 participants
@labkode
Copy link

labkode commented Feb 21, 2019

What is the purpose of this change? What does it change?

Expose internal tunnng options of the filerestorer on the restore command flags:

--average-pack-size int    average pack size in bytes (default 5242880)
--files-writer-count int   number of open files for restore (default 32)
--worker-count int        number of workers for file restore (default 8)

Was the change discussed in an issue or in the forum before?

#2101
#2074

Checklist

  • I have read the Contribution Guidelines
  • I have added tests for all changes in this PR
  • I have added documentation for the changes (in the manual)
  • There's a new file in changelog/unreleased/ that describes the changes for our users (template here)
  • I have run gofmt on the code in all commits
  • All commit messages are formatted in the same style as the other commits in the repo
  • I'm done, this Pull Request is ready for review

@labkode labkode force-pushed the labkode:restic-filerestorer-flags branch from 08e52a4 to 12fd4ad Feb 21, 2019

@labkode labkode force-pushed the labkode:restic-filerestorer-flags branch from 12fd4ad to e57e93f Feb 21, 2019

@fd0

This comment has been minimized.

Copy link
Member

fd0 commented Feb 21, 2019

Thanks for your contribution. Unfortunately it looks like you did not discuss this change in an issue before submitting it, or did I miss anything?

I'm not a fan of exposing internal values (which are internal for a reason: they can change without notice...) to users as flags. If we need to expose something, it must be a value that is clearly understandable. The values here are specific to the current restorer implementation, but once we change that for something else, the flags make no sense any more. In my opinion, exposing them it's not the right thing to do.

In general, the project's philosophy is to have a minimal user interface which works for most of the cases. Since we cannot just remove options from restic (because that'd break user scripts), we need to be very careful with what we decide to make configurable.

Users who want to experiment with the code can change the values in the code (after reading and understanding it, of course).

I'm against merging this PR, sorry about that.

@labkode labkode changed the title cmd_restore: expose filerestorer options in flags WIP cmd_restore: expose filerestorer options in flags Feb 22, 2019

@labkode

This comment has been minimized.

Copy link
Author

labkode commented Feb 22, 2019

Hi @fd0,

thanks for your comments, I understand your point of view. Would it make more sense to expose these flags as extended options -o, --option key=value so they are not exposed directly to the user? That would allow to tune the performance of restic without having to depend on a forked version of the software and correct me if I am wrong, but do extended options come with any backwards compatibility guarantee?

You did not miss it, I missed it. I have created this issue: #2182

There are many related restore performance issues that may be overcome with the tuning of these options:
https://github.com/restic/restic/issues?utf8=%E2%9C%93&q=is%3Aissue+is%3Aopen+restore+performance

For the time being we will run our forked version with different tuning parameters against our petabyte-size storage repository to understand the performance fluctuations and we will provide you our analyses. If the performance increase is very significant it will be a pity that other restic users cannot benefit from all the power of this tool.

@labkode

This comment has been minimized.

Copy link
Author

labkode commented Mar 14, 2019

@ifedorenko

This comment has been minimized.

Copy link
Contributor

ifedorenko commented Mar 14, 2019

I am not in favour of this change, to be honest. --average-pack-size and --files-writer-count will go away once #2195 is merged. And I think it is better to get rid of workerCount altogether; restorer should be able to determine number of workers based on backend connection count and available cpu cores. We may eventually need to introduce configuration parameter to control local filesystem concurrency (i.e. how many concurrent reads or writes the filesystem can tolerate), but I don't think we've reached that point yet.

@labkode

This comment has been minimized.

Copy link
Author

labkode commented Apr 16, 2019

@ifedorenko I close this PR then.

@labkode labkode closed this Apr 16, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.