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

[Christian Ebner]

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.

But still wide open for other suggestions!