From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [IPv6:2a0f:8001:1:32::40]) by lore.proxmox.com (Postfix) with ESMTPS id 0CB961FF0C1 for ; Wed, 26 Aug 2026 14:01:18 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 568A3213E6; Wed, 26 Aug 2026 14:01:17 +0200 (CEST) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Wed, 26 Aug 2026 14:01:13 +0200 Message-Id: Subject: Re: [PATCH datacenter-manager v2 16/20] api-cache: add wrapper type From: "Lukas Wagner" To: "Dominik Csapak" , "Lukas Wagner" , X-Mailer: aerc 0.21.0-0-g5549850facc2-dirty References: <20260820145220.418032-1-l.wagner@proxmox.com> <20260820145220.418032-17-l.wagner@proxmox.com> <20fd1678-99b3-44d4-917d-f27d519bef0d@proxmox.com> In-Reply-To: X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1787745665292 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.627 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: QX4H2TCSR4XOL7MTQAZ2WXHAYYSVOW3J X-Message-ID-Hash: QX4H2TCSR4XOL7MTQAZ2WXHAYYSVOW3J X-MailFrom: l.wagner@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox Datacenter Manager development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: 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. >>=20 >> Sorry, I'll try to add additional justification in the commit message in >> the next iteration. >>=20 >> 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. >>=20 >> 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. >>=20 >> 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= =20 > have a wrapper struct. > > not sure if that's practical though ^^