Cache koji builds #748
Cache koji builds #748
Conversation
in order to decrease the load on koji. With what's currently in the pipe, we decrease the number of koji calls from 65 to 37 with the cache. Signed-off-by: Gaëtan Lehmann <gaetan.lehmann@vates.tech>
|
It would be great if it could be validated quickly—this might have an impact on koji, but only once merged 🙂 |
| def get_koji_build(session, build_id) -> dict: | ||
| cache_key = f'koji-build-{build_id}' | ||
| if not args.re_cache and cache_key in CACHE: | ||
| return cast(dict, CACHE[cache_key]) |
There was a problem hiding this comment.
the cast looks necessary just because CACHE itself cannot be properly type-hinted?
There was a problem hiding this comment.
thinking twice about it, CACHE could be typed using a TypeDict?
There was a problem hiding this comment.
I don't think it would fit well.
I could assert isinstance(…) instead of casting if you prefer
| else: | ||
| build = session.getBuild(build_id) | ||
| CACHE.set(cache_key, build, expire=RETENTION_TIME) | ||
| return build |
There was a problem hiding this comment.
instead of managing such a cache manually, wouldn't get_koji_build = functools.cache(session.getBuild) do the job? I guess other uses of CACHE could also go that same way.
There was a problem hiding this comment.
that's a persistent cache, using diskcache. I don't think functool has anything similar.
There was a problem hiding this comment.
Ah, right. Looks like this is a common pattern, though, and there are some libs for this, like:
There was a problem hiding this comment.
there is a memoize() decorator in diskcache, but it doesn't fit well with the command line option to refresh the cache
There was a problem hiding this comment.
or maybe I can set the expire property after creating the CACHE object. I'll check
There was a problem hiding this comment.
Apparently, I can't do that. I would have loved to use CACHE.memoize(), but it really doesn't fit well with the cache customization from the CLI.
|
What's the status? Does this need a re-review by @ydirson, or changes prior to this? |
4 participants
in order to decrease the load on koji. With what's currently in the pipe, we decrease the number of koji calls from 65 to 37 with the cache.