Re: [pbs-devel] [PATCH proxmox-backup v3] api: make prune-group a real workertask

["Gabriel Goller" <g.goller@proxmox.com>]

On Tue Mar 12, 2024 at 11:30 AM CET, Stefan Lendl wrote:
>
> LGTM with a slight note below for a style preference.
>
> > +    let prune_group = move |worker: Arc<WorkerTask>| {
> > +        if keep_all {
> > +            task_log!(worker, "No prune selection - keeping all files.");
> > +        } else {
>
> You don't actually need to spawn a task if you're not doing any work.
> On the other hand it makes the code slightly simpler and you need the
> worker, right?

IMO starting a worker is better, because the user sees that something is
happening... I mean we could also immediately return and show a popup in
the frontend, but that won't work when the user isn't logged
in/currently looking at the page. This way he simply sees a prune job in
the task log which prints 'No prune selection - keeping all files'.

> > [snip]
> > -    for (info, mark) in prune_info {
> > -        let keep = keep_all || mark.keep();
>
> > +        for (info, mark) in prune_info {
> > +            let keep = keep_all || mark.keep();
> > +            let backup_dir = &info.backup_dir;
>
> You wouldn't have to handle keep_all here.
>
> You're handling dry_run separatly already as well.

Hmm I think I need it here though, note that this is not inside the
if/else statement above. Even if we have keep_all we still want to go
through all the snapshots and mark them (+ output mark) I guess.

Thanks for the review!