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

[Dominik Csapak]

On 3/17/25 14:17, Christian Ebner wrote:
> On 3/17/25 14:01, Dominik Csapak wrote:
>> 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 ?
> 
> Yes, making this hidden would preserve the state of the other direction, in contrast to clearing it 
> as is the case for the current patch. But it might make the code even cleaner, as the conditional 
> switching of the values is not required anymore.
> 
>>
>>>
>>> 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...
> 
> Yes, again requiring some additional logic to handle things differently.
> 
>> 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.
> 
> I do agree, but that is what I tried to achieve in the patch already (except for preserving the rate 
> limit in the other direction)? Adding the `rate` parameter would only help to reduce the switching 
> logic based on sync direction.
> 
> But given that, I would opt for keeping the current `rate-in/out` parameters in the config, having 2 
> input fields in the UI, one however hidden from the user to avoid unnecessary confusion and preserve 
> the state of both, in-depended of sync direction.
> 
> Any objections?
> 
> In any case: Thanks a lot for the discussion!


No, I think having one field exposed to the user is an OK solution for now.

thanks!