feature: Bounded TTL Caching#508
Conversation
Make sure the TtlCache always has an upper bound on the items it can contain. Breaking changes: - No longer support per item TTL warning is emitted upon usage. - Boundary semantics are different at the exact boundary the element is expired. We don't rely on second accuracy and the new behavior is safer.
tests: make sure time is virtual for user auth caching tests
soxofaan
left a comment
soxofaan
left a comment
There was a problem hiding this comment.
the complexity of this PR is a bit bigger than I expected, especially because it introduces some breaking of the original API
maybe it's just easier to leave the original TtlCache as is, and just introduce a new BoundedTtlCache class?
| self, default_ttl: float = 60, _clock: Callable[[], float] = time.time | ||
| self, | ||
| default_ttl: float = 60, | ||
| *, |
There was a problem hiding this comment.
also make default_ttl a keyword-only argument
There was a problem hiding this comment.
And it also doesn't make sense anymore to call it default_ttl: should just be ttl
| default_ttl: float = 60, | ||
| *, | ||
| max_size: int = 1000, | ||
| _clock: Callable[[], float] = time.time, |
There was a problem hiding this comment.
Given that the tests now use time_machine to time mocking, is it still necessary to have this _clock attribute (which is only there for testability)?
|
I wouldn't mind a BoundedTtlCache then it is easier to address comments like signature changes but I'd still deprecate the old class because otherwise if issues are encountered down the road there are 2 implementations to maintain and the replacement protects more against mistakes. Can as well be 2 separate steps. Add the class and "switch usage and deprecate old class" that way deployment would be staightforward and rollback would be easy. |
|
|
||
|
|
||
| def test_bearer_auth_oidc_caching(app, requests_mock, oidc_provider): | ||
| def test_bearer_auth_oidc_caching(app, requests_mock, oidc_provider, simple_time): |
There was a problem hiding this comment.
I think using the "fast_sleep" fixture here would be more to the point and more self-explanatory
There was a problem hiding this comment.
I did is to be consistent with OpenEO Python client. The problem with fast_sleep is that it does not set a reference time. So simple_time does time_machine.move_to(1000, tick=False) and then allows fast_sleep. If you just use fixture fast_sleep still need to initialize time_machine prior to using
|
|
||
|
|
||
| def test_userinfo_url_caching(app, requests_mock, oidc_provider): | ||
| def test_userinfo_url_caching(app, requests_mock, oidc_provider, simple_time): |
|
another way to simplify this task is maybe first to eliminate the per- And then the integration with cachetool will be a lot cleaner of a PR |
|
If class is to be separated that would be #509 |
2 participants
Make sure the TtlCache always has an upper bound on the items it can contain.
Breaking changes: