[fix] Do not log an expected concurrent-clear key miss as an error - #160
Conversation
CLA Signature Passhuniu20, thanks for your pull request. All authors of the commits have signed the CLA. 👍 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 791f4488ae
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| f"{response_msg.body.get('message', 'Unknown error')}" | ||
| ) | ||
| message = response_msg.body.get("message", "Unknown error") | ||
| error_type = StorageKeyNotFoundError if KEY_NOT_FOUND_MARKER in message else RuntimeError |
There was a problem hiding this comment.
Use a structured key-miss discriminator
When any non-key GET_ERROR message contains TQKeyNotFound, this substring check reconstructs it as StorageKeyNotFoundError; for example, an unknown-field error includes both the requested and available field names, so a supported field named TQKeyNotFound causes a genuine schema error to be logged at DEBUG and potentially swallowed by callers tolerating concurrent clears. Put a dedicated error code in the reply body and inspect that field instead of matching human-readable message text.
AGENTS.md reference: AGENTS.md:L5-L9
Useful? React with 👍 / 👎.
A key can be cleared between kv_retrieve_meta and get_data, so a read may reach a storage unit whose key is gone. This is reachable through the public API and currently emits three ERROR lines, and the caller can only detect it by matching on message text. Raise StorageKeyNotFoundError (a KeyError) for it, log it at DEBUG, and report the storage unit count instead of the full routing list. Every other failure keeps its ERROR level. Signed-off-by: huniu20 <huniumail@gmail.com>
791f448 to
7592a54
Compare
CLA Signature Passhuniu20, thanks for your pull request. All authors of the commits have signed the CLA. 👍 |
Motivation
kv_batch_getresolves metadata and then fetches data, holding no lease acrossthe two steps:
A
kv_clearfrom another client can land in between, soget_datareaches astorage unit whose key is gone. Both are public APIs and TransferQueue is built
for "fine-grained, concurrent data read/write operations", so concurrent readers
and cleaners are the advertised use case.
Reproducing this on current
main— one process clears the keys another alreadyresolved metadata for:
Three ERROR lines across two processes for one expected event. Two problems:
ERROR it is indistinguishable from a real storage fault and trips log-based
alerting. The controller already treats the analogous case as a warning:
logger.warning(f"Partition {partition_id} were not found in controller!").RuntimeErrorwhosemessage is assembled across a ZMQ hop, so the only way to tolerate the race is
substring matching on the message, which breaks when the wording changes.
The third line also logs the whole routing table. With many storage units that is
hundreds of ids on one line, burying the unit, key and field the error already
names.
Modification
StorageKeyNotFoundError(KeyError), raised byStorageUnitData.get_datawhen a global index is absent.
GET_ERRORreply so the manager can rebuild the type after the ZMQ hoprather than flattening it into
RuntimeError.ERROR level and message; an unknown field still raises
ValueErrorat ERROR,since that is a caller bug rather than the race.
storage_units={list(routing.keys())}withnum_storage_units={len(routing)}.StorageKeyNotFoundErrorfromtransfer_queue.Test
tests/test_storage_key_race.py: a cleared key raisesStorageKeyNotFoundError;it is still a
KeyError; an unknown field still raisesValueError; a survivingkey is unaffected; and
_handle_getmarks theGET_ERRORreply while emitting noERROR record.