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

feat(backups): write and merge block concurrency are now configurable #6416

Merged
merged 3 commits into from
Sep 16, 2022

Conversation

fbeauchamp
Copy link
Collaborator

@fbeauchamp fbeauchamp commented Sep 12, 2022

Check list

Check if done, if not relevant leave unchecked.

  • PR reference the relevant issue (e.g. Fixes #007 or See xoa-support#42)
  • if UI changes, a screenshot has been added to the PR
  • documentation updated
  • CHANGELOG.unreleased.md:
    • enhancement/bug fix entry added
    • list of packages to release updated (${name} v${new version})
  • I have tested added/updated features (and impacted code)

Process

  1. create a PR as soon as possible
  2. mark it as WiP: (Work in Progress) if not ready to be merged
  3. when you want a review, add a reviewer (and only one)
  4. if necessary, update your PR, and re- add a reviewer

From the Four Agreements:

  1. Be impeccable with your word.
  2. Don't take anything personally.
  3. Don't make assumptions.
  4. Always do your best.

@fbeauchamp fbeauchamp force-pushed the feat_add_setting_merge_concurrency branch from 95d39df to 996e538 Compare September 13, 2022 06:37
@fbeauchamp fbeauchamp requested review from julien-f and removed request for julien-f September 13, 2022 06:38
Comment on lines +91 to +103
# settings when using Vhd directories ( s3 , encryption )
# you should use 'none' if your fs is already compressed
# changing this setting will generate new full backups
vhdDirectoryCompression = 'brotli'

# how many block can be merged in parallel per backup running
# increase to increase performance, reduce if you have timeout during merge
mergeBlockConcurrency = 2

# how many block can be uploaded in parallel
# increase to in rease performance, reduce if you have timeout or memory error during transfer
writeBlockConcurrency = 16

Copy link
Member

Choose a reason for hiding this comment

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

As these settings are for a libraries and are used in two different apps (xo-server & xo-proxy), I feel like we should provide defaults values and document them somewhere else.

I think I will merge your PR as it is and then move them directly in a centralized, would that be ok for you?

Copy link
Member

Choose a reason for hiding this comment

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

Is "2" enough as default? I think we tried with 4 with success and it was "fast enough" vs 2 (when we tested on Cécile's S3). Or maybe it was 2, I don't remember exactly 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@julien-f there is default values in the code and documentation in the config file seems like a good start. Where do you want to put the default and the documentation ?

@olivierlambert we tested 4 , but the results came after the patch release.

Copy link
Member

Choose a reason for hiding this comment

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

Should we change that to 4 then?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think we should let this as is in this PR that only exposez existing values
It will be easier to have some users test it by changing the configuration and optimize the default values later.

Copy link
Member

Choose a reason for hiding this comment

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

@fbeauchamp the config file is not good, because it's in xo-server and not in the lib that really uses these entries. Also, this does not make the settings easily discoverable for xo-proxy.

I think a dedicated @xen-orchestra/backups/config.js file would be a good first answer.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, good idea

Co-authored-by: Julien Fontanet <julien.fontanet@isonoe.net>
@julien-f julien-f changed the title feat(backup): make merge block concurrency and write block concurrency configurable feat(backups): write and merge block concurrency are now configurable Sep 16, 2022
@julien-f julien-f merged commit 9da65b6 into master Sep 16, 2022
@julien-f julien-f deleted the feat_add_setting_merge_concurrency branch September 16, 2022 12:54
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.

None yet

3 participants