Skip to content

Store users locally to improve performance when querying labs/nodes#199

Open
tmikuska wants to merge 1 commit intodevfrom
user-management-improvements
Open

Store users locally to improve performance when querying labs/nodes#199
tmikuska wants to merge 1 commit intodevfrom
user-management-improvements

Conversation

@tmikuska
Copy link
Collaborator

@tmikuska tmikuska commented Mar 7, 2026

Fixes #153

self._users_by_id = {user["id"]: user for user in user_list}
self._last_sync_users_time = time.time()

def sync_users_if_outdated(self) -> None:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok. I disliked this design before, and I dislike it now as well. I feel like this is only warranted in cases where sync method requires some additional stuff to do, if its a simple TTL mechanism, I feel we would be better off with memoization/cache.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think using TTLCache (from cachetools) or similar memoization object should be sufficient... we can create wrappers and separate TTLCache for each feature that would require its own cache and set TTL to 1+ seconds

user_cache = cachetools.TTLCache(maxsize=1, ttl=1) 
...
class user_management:

    @cached(cache=user_cache)
    def users(self):
        url = self._url_for("users")
        return self._session.get(url).json()

    def sync_users(self):
        user_cache.clear()

its just an idea... implementing same method for dozen of managers/features seems very redundant

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like it either but it works and I just went with what is already proven to work. This could be refactored but it should be done across the code and imo it's not a priority.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants