Re: [PATCH proxmox v5 13/13] s3-client: implement request counter threshold and exceeding callback

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

On 3/16/26 11:35 AM, Hannes Laimer wrote:
> On 2026-03-13 15:56, Christian Ebner wrote:
>> On 3/13/26 3:26 PM, Hannes Laimer wrote:
>>> On 2026-03-11 14:09, Christian Ebner wrote:
>>>> Allows to set the threshold values to trigger exceeding callbacks
>>>> for the s3 request counters.
>>>> The globally settable callback function is executed whenever one of
>>>> the request counters is exceeding it's set threshold value, but
>>>> only once when passing over the threshold.
>>>>
>>>> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
>>>> ---
>>>
>>> [..]
>>>
>>>> +
>>>> +    /// Update the callback executed when one of the set thresholds
>>>> is exceeded
>>>> +    pub fn update_thresholds_exceeded_callback(callback:
>>>> ThresholdExceededCallback) {
>>>> +        SHARED_COUNTER_THRESHOLD_EXCEEDED_CALLBACK
>>>> +            .write()
>>>> +            .unwrap()
>>>> +            .replace(callback);
>>>> +    }
>>>
>>> this is called on every datastore load, so
>>> `SHARED_COUNTER_THRESHOLD_EXCEEDED_CALLBACK` only holds the callback
>>> associated with the latest datastore that was looked-up. Every call
>>> would replace whatever callback was there before, at least if we don't
>>> hit the cache...
>>
>> Yes that is true, might be better to only set it once. The callback is
>> however shared behind an Arc for all datastores and already should use
>> the correct counters and threshold for the given datastore, so the
>> notifications are send for that, independent of which was the last store
>> to be instantiated since not cached.
>>
>> But it makes sense to avoid updating the function pointer each time.
>>
> 
> right, but isn't the datastore's name captured when we assign the
> callback?
> 
> ```
> +
> SharedRequestCounters::update_thresholds_exceeded_callback(Box::new(
> +                    move |counter_id, threshold, value| {
> +                        thresholds_exceeded_callback.map(|cb| {
> +                            if let Err(err) = cb(&config.name,
> counter_id, threshold, value) {
> +                                log::error!("failed to send
> notification: {err:?}");
> +                            }
> +                        });
> +                    },
> +                ));
> ```
> am I missing something?

You are right, that has to be passed in as parameter as well!