Re: [pdm-devel] [PATCH proxmox-datacenter-manager 04/15] task cache: remove max-age machanism

From: Thomas Lamprecht <t.lamprecht@proxmox.com>
To: Proxmox Datacenter Manager development discussion
	<pdm-devel@lists.proxmox.com>,
	Lukas Wagner <l.wagner@proxmox.com>
Subject: Re: [pdm-devel] [PATCH proxmox-datacenter-manager 04/15] task cache: remove max-age machanism
Date: Thu, 30 Jan 2025 17:06:47 +0100	[thread overview]
Message-ID: <77ddf1e6-38ef-4bac-9043-26a41757cfa3@proxmox.com> (raw)
In-Reply-To: <9817b471-10b5-44a4-9468-23cdb35226ef@proxmox.com>

Am 30.01.25 um 09:01 schrieb Lukas Wagner:
> 
> 
> On  2025-01-29 19:27, Thomas Lamprecht wrote:
>> Am 28.01.25 um 13:25 schrieb Lukas Wagner:
>>> This commit removes the time-based caching policy for remote tasks. It
>>> will be replaced by another cache replacement policy based on total
>>> number of tasks in an upcoming commit.
>>
>> high-level: Such commits really should state a rationale with some
>> background over why this approach has to be replaced. Noting that in
>> the cover letter too would also be appreciated, such things help to
>> "sell" series/PRs and having the underlying goal and/or pain points
>> spelled out, even if quite obvious, ensures everyone is on the same
>> page.
>>
>> Similar comment for the next patch adding the FIFO replacement policy,
>> I won't write a separate mail for that.
>>
> 
> Ah yeah sure, sorry about that.
> 
> Dominik (and Dietmar briefly as well) suggested this approach to me and after some thoughts
> I agreed this was better. Since this came from 'higher up', the 'why' was somewhat settled,
> at least in my head, and I guess that's why I kinda forgot to explain it in the commit message.
> Of course you are a 100% correct, the rationale should be included in the commit log for future reference.
> I'll wait for further reviews and then try to expand the commit messages for a v2, if that's alright with you.

Maybe reply with some rationale to the cover-letter now or the like?
I had a quick chat with Wolfgang and the "why?" question popped up there
almost immediately. As tbh., currently, without looking all to deep and
thinking all too much about this (so good chance of oversight) I only
see one benefit: having a fixed guaranteed limit of cache size (scaling
with remote count?). And I mean sure, if this would be big data, but
it essentially should be UPID + optional endtime and status.

So if size is an issue it might make more sense to use a type that does
not duplicates as much as TaskListItem does, the UPID alone contains
already most of that information. The frontend could even split that,
it's rather trivial, but even doing that on demand in the backend would
be OK. As this is a cache and basically fully internal using a binary
format might be also an option to consider, if size is the real reason
for this here that is.

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