Fix too-many-open-files leak in availability worker and version lookup - #21
Conversation
| if client is None: | ||
| client = BlazarClient(cloud_name) |
There was a problem hiding this comment.
good fix, I think there's a longstanding footgun where openstack "connection" objects aren't garbage collected.
| def get_version(repo_root: Path) -> Optional[str]: | ||
| """Gets the repository version for a given repo_root.""" | ||
| return filesystem.get_version(repo_root) | ||
| return filesystem.get_release_and_timestamp(repo_root).get("version") |
There was a problem hiding this comment.
I'm not sure exactly what this change does?
In the old path, it calls git_versioning.get_version(), which creates a new Repo() object on each call
and in the new path, it's calling get_release_and_timestamp, which also creates a new Repo on each call, but has a @cached decorator, so I guess it should reuse that cached object.
is that the only difference?
There was a problem hiding this comment.
it seems like nothing else calls git_versioning.get_version() after this, so we can probably just remove that method too.
There was a problem hiding this comment.
it looks like the other methods (list_versions, get_version_info) also use the same cache, but could be distinct per site/node, and fill the cache up?
we might want to use a different cache for the get_release_and_timestamp because it depends only on the repo_path.
There was a problem hiding this comment.
Good points.
- yes main reason was to use the cache
- good catch, removed
- good point, moved it to its own cache
…ove dead get_version
2 participants
or at least an attempt to fix...