Repository navigation
[fix] Raise in put when the controller rejects the written data - #197
Conversation
notify_data_update returned False both when no ACK arrived and when the controller rejected the update (e.g. a dtype conflict), and put_data and restore ignored it. A rejected kv_put returned normally while none of its fields became ready: the samples silently never reached kv_batch_get or the dataloader, and the later error pointed at kv_update(finalize=False). The controller ACK now says why it rejected an update and whether the partition still exists, and notify_data_update raises on a rejection. Unchanged: a missing ACK still returns False, since the controller may still apply it, and a partition cleared while its data was in flight only logs a warning, so a late writer does not fail. 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: 32bfb3dc41
ℹ️ 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".
| ) | ||
| # A rejected write makes the writer raise. A partition cleared while its data was in flight | ||
| # only makes the writer warn: that data is no longer wanted. | ||
| partition_exists = self._get_partition(cast(str, partition_id)) is not None |
There was a problem hiding this comment.
When a direct Ray clear_partition.remote call (the path used by the e2e fixtures) removes a partition between this lookup and update_production_status, the latter returns False for the now-missing partition while partition_exists remains True. The ACK is then classified as a controller rejection and notify_data_update raises, instead of taking the warning-only path promised for an in-flight clear; return the missing-partition status from the update itself or recheck it after a false result.
AGENTS.md reference: AGENTS.md:L17-L18
Useful? React with 👍 / 👎.
Signed-off-by: 0oshowero0 <o0shower0o@outlook.com>
0oshowero0
changed the title
put when the controller rejects the written data
Problem
A put could return normally even when the controller failed to record the written data, for example because a field's dtype conflicted with earlier writes in the partition. Storage managers write payloads before notifying the controller: a rejected new key remains unreadable, while an overwrite of an already-ready field can remain visible under stale controller metadata.
Change
REQUEST_ERRORreply. The controller request loop catches the exception, sends the error, and continues serving requests.REQUEST_ERRORreplies. Explicit controller errors raise at the writer immediately; they are no longer ignored until timeout. The error is raised outside the transport exception handler so it reaches the caller.successflag.Falsemeans the partition no longer exists, so a late writer only warns. A missing reply still returnsFalsefromnotify_data_update, because the controller may yet apply the update.kv_update's warning that a committed merge must not be retried blindly, and include the controller's error without depending on its wording.The same client can catch a dtype rejection and put corrected data without restarting the controller or client. A failed put can already have overwritten the previous value; callers must supply the correct replacement data when retrying.
Validation
python -m compileall -q transfer_queue tutorial testspasses.TQ_TEST_BACKEND=SimpleStorage: 860 passed, 12 skipped, 8 errors. The eight errors match the baseline: Yuanrong test fixtures cannot patchdatasystemwhen its optional dependency is absent.REQUEST_ERROR, accepted/missing/cleared-partition ACK behavior, the cleared-partition warning, and the committed-merge error.Mooncake and Yuanrong service integration was not run.
Found while reviewing #190.