Re: [PATCH datacenter-manager v2 16/20] api-cache: add wrapper type

["Lukas Wagner" <l.wagner@proxmox.com>]

On Mon Aug 24, 2026 at 3:57 PM CEST, Dominik Csapak wrote:
>
>
> On 8/24/26 3:46 PM, Lukas Wagner wrote:
>> On Mon Aug 24, 2026 at 2:33 PM CEST, Dominik Csapak wrote:
>>> i'm missing a bit here why we need a wrapper around a type we fully
>>> control here? couldn't we use NamespacedCache directly?
>>>
>>> what advantage has wrapping it in this way?
>>>
>>> To me it's not immediately obvious, so a short sentence
>>> in the commit message or as comment on the struct, would be good.
>> 
>> Sorry, I'll try to add additional justification in the commit message in
>> the next iteration.
>> 
>> NamespacedCache is supposed to be fully generic and eventually be moved
>> to a shared crate. ApiCache adds PDM-specific semantics, namely having
>> per-remote namespaces and also one global namespace.
>> 
>> Before, these additional semantics were encoded in the helper functions
>> in api_cache.rs (e.g. read_global, read_remote), but since we want to
>> put this thing in PdmApplication, the helpers are turned into methods of
>> this new wrapper type.
>> 
>> Hope with this explanation it makes more sense?
>
> yes that makes the patch a bit clearer, thanks for explaining
>
> we could still think about building the namespacing into the
> underlying generic cache struct though,

NamespacedCache is, as the name implies, already namespaced, meaning
that whenever you want to read or write a value, you have to first
provide it the namespace, right now this is done when acquiring the read
or write lock, as the namespace as a whole gets locked.

ApiCache just uses these namespaces to offer semantics better suited to
caching API responses; that is namespaces for individual remotes as well
as a global one for aggregations for results from multiple remotes.

If you do a read_remote(remote: &str), under the hood it will use the
namespace 'remote-{remote}', and read_global just uses a hard-coded
'global' namespace.

Admitted, it adds a bit of boilerplate, but I think it makes it much
more convenient to use for callers.

Anyways, we can still change the approach later, this is just the
smallest change needed to let the API cache be owned by the application
context object.

I've extended the commit message in the upcoming v3 to avoid further
confusion.

Thanks!

>
> e.g we could have a trait
>
> CachePath {
>     fn create_cache_path(&self, input: String) -> String
> }
>
> (or similar)
>
> which can be a noop by default, and e.g. pdm can implement a
>
> `NamespacedCache`
>
> that implements it differently
>
>
> then caller can call directly into the cache object and we don't have to 
> have a wrapper struct.
>
> not sure if that's practical though ^^