feat: Caching URL and account info #385
Conversation
src/firebolt/async_db/util.py
Outdated
| from firebolt.utils.util import parse_url_and_params | ||
| from firebolt.utils.util import UtilCache, parse_url_and_params | ||
|
|
||
| _firebolt_async_system_engine_cache = UtilCache[Tuple[str, Dict[str, str]]]() |
There was a problem hiding this comment.
why don't we have a global cache?
There was a problem hiding this comment.
It's global, since it's initialised on import. Or do you mean like a centralised one in, for example, connection.py? What would be an advantage of that?
There was a problem hiding this comment.
I was just thinking there is no need for separate cache instances for async and sync since it's always sync. Maybe we can have in somewhere in common?
Also, in I believe in extremely rare cases users can have both sync and async runtimes simultaneously and can reuse the cache in both, but not sure if that will ever happen. The main point is just to simplify
Not at the moment. Good point, we should be able to disable cache. |
src/firebolt/async_db/util.py
Outdated
| account_name: str, | ||
| api_endpoint: str, | ||
| ) -> Tuple[str, Dict[str, str]]: | ||
| cache_key = f"{account_name}-{api_endpoint}" |
There was a problem hiding this comment.
We still create a key manually here. But I was thinking even more, just for get to have a signature def get(*key): and then inside do cache_key = self.create_key(*key) or something simillar
There was a problem hiding this comment.
Whoops, missed this one. I see where you're getting at now. My only problem with this is def set(*key, value) can lead to non-obvious errors when value is forgotten from the call, e.g. cache.set(account_name, api_endpoint) will work just fine, but lead to runtime issues. I've pushed a different approach with key accepting anything and using repr to convert to a hashable key. Hopefully it solves both issues.
|
|
Integration tests are failing on an unrelated issue with the server response to an account with no user. It should be addressed in a separate PR. |
2 participants
FIR-33386
I had to create our own cache class since the standard
lru_cachehas issues, namely: