Skip to content

Commit f809fe7

Browse files
Pijukatelclaude
andcommitted
fix: retry the batched Redis value read and test the batching helper
The default iteration retries every read through `get_value`, but the batched HMGET in the Redis override was not retried. It now returns a list under the same `retry_on_error` as the other Redis reads, matching the SQL client. Add a direct unit test for `batch_records_by_size`, covering the count bound, the size bound, an oversized record alone in its batch and records of unknown size. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Wg6jZgyQp7XkvVvxuueJyV
1 parent 1f32875 commit f809fe7

2 files changed

Lines changed: 57 additions & 8 deletions

File tree

‎src/crawlee/storage_clients/_redis/_key_value_store_client.py‎

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -302,19 +302,20 @@ async def iterate_entries(
302302
max_records=self._ITERATE_ENTRIES_BATCH_MAX_KEYS,
303303
max_bytes=self._ITERATE_ENTRIES_BATCH_MAX_BYTES,
304304
):
305-
async for record in self._fetch_records(batch):
305+
for record in await self._fetch_records(batch):
306306
yield record
307307

308-
async def _fetch_records(self, batch: list[KeyValueStoreRecordMetadata]) -> AsyncIterator[KeyValueStoreRecord]:
309-
"""Fetch the values of the given records with a single HMGET call and yield the deserialized records."""
308+
@retry_on_error(RedisError)
309+
async def _fetch_records(self, batch: list[KeyValueStoreRecordMetadata]) -> list[KeyValueStoreRecord]:
310+
"""Fetch the values of the given records with a single HMGET call and return the deserialized records."""
310311
keys = [item.key for item in batch]
311312
values = [expect_bytes(v) for v in await await_redis_response(self._redis.hmget(self._items_key, keys))]
312313

313-
for metadata_item, value_bytes in zip(batch, values, strict=True):
314-
record = self._build_record(metadata_item, value_bytes)
315-
if record is None:
316-
continue
317-
yield record
314+
records = (
315+
self._build_record(metadata_item, value_bytes)
316+
for metadata_item, value_bytes in zip(batch, values, strict=True)
317+
)
318+
return [record for record in records if record is not None]
318319

319320
@override
320321
async def get_public_url(self, *, key: str) -> str:
Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
from __future__ import annotations
2+
3+
from crawlee.storage_clients._utils import batch_records_by_size
4+
from crawlee.storage_clients.models import KeyValueStoreRecordMetadata
5+
6+
7+
def _record(key: str, size: int | None) -> KeyValueStoreRecordMetadata:
8+
return KeyValueStoreRecordMetadata(key=key, content_type='application/octet-stream', size=size)
9+
10+
11+
def _keys(batches: list[list[KeyValueStoreRecordMetadata]]) -> list[list[str]]:
12+
return [[record.key for record in batch] for batch in batches]
13+
14+
15+
def test_batches_are_bounded_by_record_count() -> None:
16+
records = [_record(f'k{i}', 1) for i in range(5)]
17+
18+
batches = list(batch_records_by_size(records, max_records=2, max_bytes=1000))
19+
20+
assert _keys(batches) == [['k0', 'k1'], ['k2', 'k3'], ['k4']]
21+
22+
23+
def test_batches_are_bounded_by_total_size() -> None:
24+
records = [_record('a', 4), _record('b', 4), _record('c', 4), _record('d', 4)]
25+
26+
batches = list(batch_records_by_size(records, max_records=100, max_bytes=10))
27+
28+
assert _keys(batches) == [['a', 'b'], ['c', 'd']]
29+
30+
31+
def test_oversized_record_is_alone_in_its_batch() -> None:
32+
records = [_record('small1', 2), _record('large', 100), _record('small2', 2), _record('small3', 2)]
33+
34+
batches = list(batch_records_by_size(records, max_records=100, max_bytes=10))
35+
36+
assert _keys(batches) == [['small1'], ['large'], ['small2', 'small3']]
37+
38+
39+
def test_unknown_size_counts_as_empty() -> None:
40+
records = [_record('a', None), _record('b', None), _record('c', 10)]
41+
42+
batches = list(batch_records_by_size(records, max_records=100, max_bytes=10))
43+
44+
assert _keys(batches) == [['a', 'b', 'c']]
45+
46+
47+
def test_no_records_yield_no_batches() -> None:
48+
assert list(batch_records_by_size([], max_records=100, max_bytes=10)) == []

0 commit comments

Comments
 (0)