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(xo-web): merge backups synchronously #8177

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

stephane-m-dev
Copy link
Contributor

@stephane-m-dev stephane-m-dev commented Dec 5, 2024

Description

[Backup] Add Merge backups synchronously to Advanced Settings
We need to introduce a new backup advanced setting: synchronousMerge

XO-356
https://project.vates.tech/vates-global/projects/70ab2907-1ac3-4e7d-831f-a8752c36474d/issues/63832d60-23c4-44dc-a9e1-462cd94565b7

Capture d’écran du 2024-12-12 02-01-59

Capture d’écran du 2024-12-13 15-07-20

Checklist

  • Commit
    • Title follows commit conventions
    • Reference the relevant issue (Fixes #007, See xoa-support#42, See https://...)
    • If bug fix, add Introduced by
  • Changelog
    • If visible by XOA users, add changelog entry
    • Update "Packages to release" in CHANGELOG.unreleased.md
  • PR
    • If UI changes, add screenshots
    • If not finished or not tested, open as Draft

Review process

This 2-passes review process aims to:

  • develop skills of junior reviewers
  • limit the workload for senior reviewers
  • limit the number of unnecessary changes by the author
  1. The author creates a PR.
  2. Review process:
    1. The author assigns the junior reviewer.
    2. The junior reviewer conducts their review:
      • Resolves their comments if they are addressed.
      • Adds comments if necessary or approves the PR.
    3. The junior reviewer assigns the senior reviewer.
    4. The senior reviewer conducts their review:
      • If there are no unresolved comments on the PR → merge.
      • Otherwise, we continue with 3.
  3. The author responds to comments and/or makes corrections, and we go back to 2.

Notes:

  1. The author can request a review at any time, even if the PR is still a Draft.
  2. In theory, there should not be more than one reviewer at a time.
  3. The author should not make any changes:
    • When a reviewer is assigned.
    • Between the junior and senior reviews.

@stephane-m-dev stephane-m-dev force-pushed the merge-backups-synchronously branch from b752b2a to 5d3c98b Compare December 5, 2024 16:32
Copy link
Contributor

@b-Nollet b-Nollet left a comment

Choose a reason for hiding this comment

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

I may be missing something, but I don't get why we use backup report templates here (mjml and compactMjml), I feel like using a boolean value and renaming mergeBackupSynchronouslyTpl into mergeBackupSynchronously would make more sense.

@stephane-m-dev
Copy link
Contributor Author

I may be missing something, but I don't get why we use backup report templates here (mjml and compactMjml), I feel like using a boolean value and renaming mergeBackupSynchronouslyTpl into mergeBackupSynchronously would make more sense.

@b-Nollet You are absolutely right. But I haven't found how to make the toggle work without that, and pass a boolean from the front side.
How can we do that?

@b-Nollet
Copy link
Contributor

b-Nollet commented Dec 6, 2024

@b-Nollet You are absolutely right. But I haven't found how to make the toggle work without that, and pass a boolean from the front side. How can we do that?

You can use toggles from other pages as example.
I think you can just use value={mergeBackupSynchronously} for the toggle instead of the current value and setGlobalSettings({mergeBackupSynchronously}) in the setMergeBackupsSynchronously function.

I haven't tested this though, and I'm not a front expert, so feel free to ask Pierre or Mathieu if in doubt.

Copy link
Collaborator

@fbeauchamp fbeauchamp left a comment

Choose a reason for hiding this comment

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

You can look at the toggle used to enable/disable NBD , specifically state.inputPreferNbd and effects.setPreferNbd to see how to handle boolean in the front

this is seen in the api and backend as the preferNbd setting

@stephane-m-dev stephane-m-dev force-pushed the merge-backups-synchronously branch from 89b96a4 to 1349679 Compare December 12, 2024 09:18
@b-Nollet b-Nollet requested a review from fbeauchamp December 12, 2024 09:39
@@ -570,6 +570,7 @@ const messages = {
reportRecipients: 'Report recipients',
reportWhen: 'Report when',
concurrency: 'Concurrency',
mergeBackupsSynchronously: 'Merge backups synchronously',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
mergeBackupsSynchronously: 'Merge backups synchronously',
mergeBackupsSynchronously: 'Merge backups synchronously. This will use more resources on the backup thread, but ensure there is no locking error when chaining multiple backup jobs on the same remote',

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@stephane-m-dev stephane-m-dev force-pushed the merge-backups-synchronously branch from 94ad837 to c358d0f Compare December 13, 2024 23:00
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.

3 participants