Repository navigation
[chore] Log each failed storage unit once in clear_data, with what it kept - #198
Conversation
A failed clear logged two lines per unit: one from _clear_single_storage_unit and one naming only the gather task index. Log one line per failed unit instead, with the unit id, the number of samples and fields it still holds, and the error. Behavior is unchanged: the failure is still logged rather than raised, so a cleanup failure does not stop the caller's job. Signed-off-by: 0oshowero0 <o0shower0o@outlook.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d1ccf4141b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| logger.error(f"[{self.storage_manager_id}]: Error in clear operation task {i}: {result}") | ||
| logger.error( | ||
| f"[{self.storage_manager_id}]: Failed to clear {len(group.global_indexes)} samples " | ||
| f"(fields={fields or 'all'}) on storage unit {su_id}; they stay in its memory: " |
There was a problem hiding this comment.
When a send/receive timeout happens after CLEAR_DATA is on the wire, the storage unit can already have completed the deletion; ZMQSocketPool.lease even discards the socket because a reply may still arrive (transfer_queue/utils/zmq_utils.py:496-497). This new message nevertheless states that the samples stay in memory, so the failure log can falsely report retained data—the key state this change is meant to expose. Describe the clear as unconfirmed/data as possibly remaining instead.
AGENTS.md reference: AGENTS.md:L5-L9
Useful? React with 👍 / 👎.
Problem
When a storage unit fails to clear,
AsyncSimpleStorageManager.clear_datalogs it and returns, sokv_clear,clear_samplesandclear_partitionstill release the controller metadata. That is intended: the caller no longer wants the data, and failing its cleanup would stop the job. Since #190 the payloads a unit keeps can no longer be read or merged into a laterkv_update. They only take memory until their indexes are reused and cleared.The log for such a failure is hard to use. Each failed unit produces two lines, and the second names only the index of the gather task:
Neither line says how much data the unit still holds.
Change
clear_datalogs one line per failed unit, with the unit, the number of samples and the fields it still holds, and the error:_clear_single_storage_unitno longer logs before re-raising, so each failure is logged once. A short comment inclear_datarecords why a failure is logged rather than raised.Behavior is unchanged: a failed unit is still not raised, and every unit is still attempted.
Tests
tests/test_async_simple_storage_manager.py::test_clear_data_routes_from_hashnow also fails one of two units. It checks thatclear_datadoes not raise, that the other unit is still cleared, and that exactly one log line names the failed unit, its sample count and the error. That part fails without the change.python -m compileall -q transfer_queue tutorial tests,ruff checkandruff format --checkpass.python -m pytest -q: 851 passed, 12 skipped. The 8 errors are intests/test_yuanrong_storage_client_e2e.pyand are environment-only; they occur onmaintoo.