Re: [pbs-devel] [PATCH proxmox-backup 0/3] backup client progress log interval

[Christian Ebner <c.ebner@proxmox.com>]

On 10/21/24 17:08, Thomas Lamprecht wrote:
> Am 21/10/2024 um 14:55 schrieb Christian Ebner:
>> These patches allow to specify a time based or size based interval
>> for progress log output as generated during `proxmox-backup-client
>> backup` runs.
>>
>> The client is extended by an optional `progress-log-interval`
>> parameter, which allows to set the interval as `TimeSpan` or
>> `HumanByte` parsable input string, depending on the variant prefix.
>> If set to `none` the progress log output is disabled, if no prefix is
>> specified, a time based interval is assumed. Lastly, if the parameter
>> is not given, the default 'time:1m' is used.
>>
> 
> nice work but I'm a bit torn w.r.t. exposing the variant inside the
> value, could be nicer to have it written out in the option name itself,
> e.g.:
> 
> --progress-interval for time based, as intervals are very often time
>    based anyway, and it's probably what most user want by default.
> 
> --progress-size-interval for size based, albeit the option name is not
>    really _that_ good, but progress-interval-size, which would be nicer
>    for choosing between both via autocompletion as they share a longer
>    prefix, has an even worse ring to it IMO. That's why I'm torn, the
>    variant as value avoids this odd option naming, but still, multiplexing
>    option is juts not that great.
> 
> What do you think?

Okay, fine by me as well. To be honest I only followed trough with the 
variant in value approach since I did already implemented most of it on 
Friday before I did see your email regarding discouraging that approach.

If going for two different optional parameters, one question remains 
however: how to handle the no logging case? Should a 0 value indicate 
that or do we want another explicit flag for that?

> Oh, and in above examples I also dropped the "log" as while it certainly
> doesn't hurt its IMO also not really required to be able to understand what
> it's for.

Agreed! Thank you for your comments.


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