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

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

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!


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