[pbs-devel] [PATCH proxmox-backup 1/2] fix: ui: sync job: switch rate limit based on sync direction

[Dominik Csapak]

On 3/17/25 13:54, Christian Ebner wrote:
> On 3/17/25 13:36, Dominik Csapak wrote:
>> On 3/17/25 13:11, Christian Ebner wrote:
>>> On 3/17/25 12:56, Dominik Csapak wrote:
>>>> On 3/17/25 11:32, Christian Ebner wrote:
>>>>> On 3/17/25 11:07, Dominik Csapak wrote:
>>>>>> High level comment:
>>>>>>
>>>>>> I know it's preexisting, bu does it even make sense to have a 'rate- in' and 'rate-out' for sync
>>>>>> jobs? would it not make more sense to have a single 'rate' parameter and apply it to both
>>>>>> directions?
>>>>>
>>>>> You mean only as additional parameter for the api endpoint for sync job config creation and 
>>>>> update? Or as parameter for the sync job config itself?
>>>>>
>>>>> The former might be the better option, and one can check if both rate and rate-in/out were set 
>>>>> and abort with error in that case or abort with error if a rate-in was configured for a push or 
>>>>> rate-out for a pull?
>>>>>
>>>>
>>>> i had actually imagined 3 options for the sync job config
>>>> rate: limits both in/out
>>>> rate-in/out: precedence over rate, limits the respective direction
>>>>
>>>> and only expose the 'rate' option on the ui
>>>
>>> Okay, that makes sense, but the issue I see there is that per-existing rate limits are not shown 
>>> in the UI anymore, as the `rate` field is now used, while the config has the explicit `rate-in/ 
>>> out` set.
>>>
>>> So this would need some merging first, or am I missing something?
>>
>> no, you're right, but i think we have a few options:
>>
>> * contrary to what i suggested, don't prioritize rate-in/out, but only use it when 'rate' is not set
>> * delete rate-in/out when setting the rate via the gui
> 
> Both of these however still require some additional logic on the config values, which would be nice 
> if one could avoid. Having only `rate-in/out` at the config level makes more sense to me, avoiding 
> any need to conditionally merge or set values.
> 
>> * as you suggested in the other response, only keep rate-in and rate-out but expose both
>>    (although i can already see the confusion about what is 'in' and what is 'out' regarding sync 
>> direction)
> 
> This possible confusion could however be reduced by adding some more context to the field label (or 
> even symbols as for the sync direction on the create sync job buttons).
> E.g. `Rate limit in (pull contents)` and `Rate limit out (push contents)`.
> 
> Another option might be to make one or the other only visible in the UI for the corresponding sync 
> direction, or render the "less" useful rate limit in the advanced options only, depending on the 
> sync jobs type.

AFAIU this is basically what you sent (minus the deletion of the other option), right ?

> 
> But still wide open for other suggestions!

even another possibility is to simply set both from the ui?
as in we have a 'rate' field on the ui that sets both rate-in and rate-out?

the only question then is how to handle if the values are different on the backend...

The problem i have with the current way, is that we need to expose the user to a setting that
is unnecessary and confusing in most of the cases. I'd really like to make this not more
complicated as it already is. Thus my suggestion to reduce it to a single 'rate' parameter.