Re: [pbs-devel] [PATCH v4 proxmox proxmox-backup 0/7] fix #4182: concurrent group pull/push support for sync jobs

["Max Carrara" <m.carrara@proxmox.com>]

On Fri Apr 4, 2025 at 3:49 PM CEST, Christian Ebner wrote:
> Syncing contents from/to a remote source via a sync job suffers from
> low throughput on high latency networks because of limitations by the
> HTTP/2 connection, as described in [0]. To improve, syncing multiple
> groups in parallel by establishing multiple reader instances has been
> suggested.
>
> This patch series implements the functionality by adding the sync job
> configuration property `parallel-groups`, allowing to define the
> number of concurrent groups pull/push futures to be instantiated and
> executed for each job.
> The property is currently not exposed on the UI, as intended to be
> set in the config directly for now.
>
> Examplary configuration:
> ```
> sync: s-8764c440-3a6c
> 	ns
> 	owner root@pam
> 	remote local
> 	remote-ns
> 	remote-store push-target-store
> 	remove-vanished false
> 	store datastore
> 	sync-direction push
> 	parallel-groups 4
> ```
>
> Since log messages are now also written concurrently, prefix logs
> related to groups, snapshots and archives with their respective
> context prefix and add context to error messages.
>
> Further, improve logging especially for sync jobs in push direction,
> which only displayed limited information so far.
>
> [0] https://bugzilla.proxmox.com/show_bug.cgi?id=4182

So, I've given the code a good look -- unfortunately it's too late to do
any additional testing, but I wanted to shoot this out regardless in the
meantime.

Code Review
===========

As always, the code quality is pristine -- I like that you're factoring
things out into little helper functions where applicable instead of
letting the existing methods grow. Very nice. Also applies cleanly
and is formatted correctly, naturally. Really can't complain, the
changes are straightforward and easy to follow.

There's only a couple little things I spotted; see my comments inline.

Regarding that large comment about mutexes and atomics: That's something
I just wanted to mention, so just to make it clear, you don't need to
apply my suggestion :P It's probably something we should have a look at
tree-wide for other data structures, too.

Splendid work as always, anyhow!

For now, until I get to test this, consider:

Reviewed-by: Max Carrara <m.carrara@proxmox.com>

>
> proxmox:
>
> Christian Ebner (1):
>   pbs api types: add 'parallel-groups' to sync job config
>
>  pbs-api-types/src/jobs.rs | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> proxmox-backup:
>
> Christian Ebner (6):
>   client: backup writer: fix upload stats size and rate for push sync
>   api: config/sync: add optional `parallel-groups` property
>   fix #4182: server: sync: allow pulling groups concurrently
>   server: pull: prefix log messages and add error context
>   server: sync: allow pushing groups concurrently
>   server: push: prefix log messages and add additional logging
>
>  pbs-client/src/backup_stats.rs      |  20 +--
>  pbs-client/src/backup_writer.rs     |   4 +-
>  pbs-datastore/src/store_progress.rs |   2 +-
>  src/api2/config/sync.rs             |  10 ++
>  src/api2/pull.rs                    |   9 +-
>  src/api2/push.rs                    |   9 +-
>  src/server/pull.rs                  | 209 +++++++++++++++++-----------
>  src/server/push.rs                  | 147 ++++++++++++++-----
>  src/server/sync.rs                  |  10 +-
>  9 files changed, 287 insertions(+), 133 deletions(-)



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel