Skip to content

Commit 6a9455f

Browse files
Refactor MemoryCacheEntry to use expires_at instead of ttl_at_insert
This refactoring addresses CodeRabbit's feedback by: - Removing the ttl_at_insert field from MemoryCacheEntry - Storing expires_at directly instead of calculating it on-the-fly - Eliminating the side effect mutation in _memory_cache_ttu - Fixing the stale TTL bug where recalculate_ttl() wouldn't work - Simplifying the code with clearer semantics Benefits: - No more problematic side effects - Fixes TTL recalculation bug automatically - Clearer code: we store what we actually use (expires_at) - Less indirection between TTL and expires_at Co-authored-by: William Easton <[email protected]>
1 parent 90fd2c8 commit 6a9455f

File tree

2 files changed

+20
-43
lines changed
  • key-value
    • key-value-aio/src/key_value/aio/stores/memory
    • key-value-sync/src/key_value/sync/code_gen/stores/memory

2 files changed

+20
-43
lines changed

key-value/key-value-aio/src/key_value/aio/stores/memory/store.py

Lines changed: 10 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,9 @@
11
import sys
2-
from dataclasses import dataclass, field
2+
from dataclasses import dataclass
33
from datetime import datetime
4-
from typing import Any, SupportsFloat
4+
from typing import Any
55

66
from key_value.shared.utils.managed_entry import ManagedEntry
7-
from key_value.shared.utils.time_to_live import epoch_to_datetime
87
from typing_extensions import Self, override
98

109
from key_value.aio.stores.base import (
@@ -27,30 +26,26 @@ class MemoryCacheEntry:
2726
"""A cache entry for the memory store.
2827
2928
This dataclass represents an entry in the MemoryStore cache, storing the JSON-serialized
30-
value along with expiration metadata and the original TTL value at insertion time.
29+
value along with its expiration timestamp.
3130
"""
3231

3332
json_str: str
3433

3534
expires_at: datetime | None
3635

37-
ttl_at_insert: SupportsFloat | None = field(default=None)
38-
3936
@classmethod
40-
def from_managed_entry(cls, managed_entry: ManagedEntry, ttl: SupportsFloat | None = None) -> Self:
37+
def from_managed_entry(cls, managed_entry: ManagedEntry) -> Self:
4138
"""Create a cache entry from a ManagedEntry.
4239
4340
Args:
4441
managed_entry: The ManagedEntry to convert.
45-
ttl: The TTL value at insertion time.
4642
4743
Returns:
4844
A new MemoryCacheEntry.
4945
"""
5046
return cls(
5147
json_str=managed_entry.to_json(),
5248
expires_at=managed_entry.expires_at,
53-
ttl_at_insert=ttl,
5449
)
5550

5651
def to_managed_entry(self) -> ManagedEntry:
@@ -62,28 +57,23 @@ def to_managed_entry(self) -> ManagedEntry:
6257
return ManagedEntry.from_json(json_str=self.json_str)
6358

6459

65-
def _memory_cache_ttu(_key: Any, value: MemoryCacheEntry, now: float) -> float:
66-
"""Calculate time-to-use for cache entries based on their TTL.
60+
def _memory_cache_ttu(_key: Any, value: MemoryCacheEntry, now: float) -> float: # noqa: ARG001
61+
"""Calculate time-to-use for cache entries based on their expiration time.
6762
6863
This function is used by TLRUCache to determine when entries should expire.
69-
It calculates the expiration timestamp and updates the entry's expires_at field.
7064
7165
Args:
7266
_key: The cache key (unused).
7367
value: The cache entry.
74-
now: The current time as an epoch timestamp.
68+
now: The current time as an epoch timestamp (unused).
7569
7670
Returns:
7771
The expiration time as an epoch timestamp, or sys.maxsize if the entry has no TTL.
7872
"""
79-
if value.ttl_at_insert is None:
73+
if value.expires_at is None:
8074
return float(sys.maxsize)
8175

82-
expiration_epoch: float = now + float(value.ttl_at_insert)
83-
84-
value.expires_at = epoch_to_datetime(epoch=expiration_epoch)
85-
86-
return float(expiration_epoch)
76+
return value.expires_at.timestamp()
8777

8878

8979
def _memory_cache_getsizeof(value: MemoryCacheEntry) -> int: # noqa: ARG001
@@ -153,9 +143,7 @@ def put(self, key: str, value: ManagedEntry) -> None:
153143
key: The key to store under.
154144
value: The ManagedEntry to store.
155145
"""
156-
# Recalculate TTL to get remaining time until expiration
157-
value.recalculate_ttl()
158-
self._cache[key] = MemoryCacheEntry.from_managed_entry(managed_entry=value, ttl=value.ttl)
146+
self._cache[key] = MemoryCacheEntry.from_managed_entry(managed_entry=value)
159147

160148
def delete(self, key: str) -> bool:
161149
"""Delete an entry from the collection.

key-value/key-value-sync/src/key_value/sync/code_gen/stores/memory/store.py

Lines changed: 10 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -2,12 +2,11 @@
22
# from the original file 'store.py'
33
# DO NOT CHANGE! Change the original file instead.
44
import sys
5-
from dataclasses import dataclass, field
5+
from dataclasses import dataclass
66
from datetime import datetime
7-
from typing import Any, SupportsFloat
7+
from typing import Any
88

99
from key_value.shared.utils.managed_entry import ManagedEntry
10-
from key_value.shared.utils.time_to_live import epoch_to_datetime
1110
from typing_extensions import Self, override
1211

1312
from key_value.sync.code_gen.stores.base import (
@@ -30,27 +29,24 @@ class MemoryCacheEntry:
3029
"""A cache entry for the memory store.
3130
3231
This dataclass represents an entry in the MemoryStore cache, storing the JSON-serialized
33-
value along with expiration metadata and the original TTL value at insertion time.
32+
value along with its expiration timestamp.
3433
"""
3534

3635
json_str: str
3736

3837
expires_at: datetime | None
3938

40-
ttl_at_insert: SupportsFloat | None = field(default=None)
41-
4239
@classmethod
43-
def from_managed_entry(cls, managed_entry: ManagedEntry, ttl: SupportsFloat | None = None) -> Self:
40+
def from_managed_entry(cls, managed_entry: ManagedEntry) -> Self:
4441
"""Create a cache entry from a ManagedEntry.
4542
4643
Args:
4744
managed_entry: The ManagedEntry to convert.
48-
ttl: The TTL value at insertion time.
4945
5046
Returns:
5147
A new MemoryCacheEntry.
5248
"""
53-
return cls(json_str=managed_entry.to_json(), expires_at=managed_entry.expires_at, ttl_at_insert=ttl)
49+
return cls(json_str=managed_entry.to_json(), expires_at=managed_entry.expires_at)
5450

5551
def to_managed_entry(self) -> ManagedEntry:
5652
"""Convert this cache entry to a ManagedEntry.
@@ -62,27 +58,22 @@ def to_managed_entry(self) -> ManagedEntry:
6258

6359

6460
def _memory_cache_ttu(_key: Any, value: MemoryCacheEntry, now: float) -> float:
65-
"""Calculate time-to-use for cache entries based on their TTL.
61+
"""Calculate time-to-use for cache entries based on their expiration time.
6662
6763
This function is used by TLRUCache to determine when entries should expire.
68-
It calculates the expiration timestamp and updates the entry's expires_at field.
6964
7065
Args:
7166
_key: The cache key (unused).
7267
value: The cache entry.
73-
now: The current time as an epoch timestamp.
68+
now: The current time as an epoch timestamp (unused).
7469
7570
Returns:
7671
The expiration time as an epoch timestamp, or sys.maxsize if the entry has no TTL.
7772
"""
78-
if value.ttl_at_insert is None:
73+
if value.expires_at is None:
7974
return float(sys.maxsize)
8075

81-
expiration_epoch: float = now + float(value.ttl_at_insert)
82-
83-
value.expires_at = epoch_to_datetime(epoch=expiration_epoch)
84-
85-
return float(expiration_epoch)
76+
return value.expires_at.timestamp()
8677

8778

8879
def _memory_cache_getsizeof(value: MemoryCacheEntry) -> int:
@@ -148,9 +139,7 @@ def put(self, key: str, value: ManagedEntry) -> None:
148139
key: The key to store under.
149140
value: The ManagedEntry to store.
150141
"""
151-
# Recalculate TTL to get remaining time until expiration
152-
value.recalculate_ttl()
153-
self._cache[key] = MemoryCacheEntry.from_managed_entry(managed_entry=value, ttl=value.ttl)
142+
self._cache[key] = MemoryCacheEntry.from_managed_entry(managed_entry=value)
154143

155144
def delete(self, key: str) -> bool:
156145
"""Delete an entry from the collection.

0 commit comments

Comments
 (0)