docs: add Idempotent Requests user guide - #3635
Conversation
Add a user guide for the idempotency key: how to send it over REST and gRPC, the rules that hold for every operation, and the per-operation behavior for insert (milvus-io/milvus#50007) and bulk import (milvus-io/milvus#52544), followed by the related configuration. Register the page under Insert & Delete in the menu, and point to it from Insert Entities and Import Data. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQihG8uqLQb8LWW5aVivjY Signed-off-by: bigsheeper <yihao.dai@zilliz.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: bigsheeper The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
This documentation review of the new idempotent-requests page found one high-severity gap and several accuracy issues, all introduced by this PR, with the most important being that the page does not warn readers that keyless inserts are content-hash deduplicated once idempotent insert is enabled. High
Medium
Low
|
Corrections from the review of this page: - A keyless insert is content-deduplicated once idempotent insert is enabled, so "a request without a key behaves exactly as before" was false in that configuration. Scope the claim and add a note that two intentionally distinct inserts with identical payloads collapse into one unless each carries its own explicit key. - Turning off the global switch and restarting discards every stored insert record, which the window description and the config table did not say. - The Go SDK rejects a key on Upsert rather than ignoring it. - A failed original usually releases the key, but a message queue that persists a write while reporting an error can let a retry write twice. - 0 for maxKeyLength rejects every key at the REST door and the coordinator, while the proxy insert path reads it as unbounded. - The bound is at most 256 bytes, not under 256. - Field order does not affect the automatic key; fields are sorted before hashing. - The default import retention covers one StreamingCoord restart per tombstone lifetime rather than every case. - State that an older SDK sends no key. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQihG8uqLQb8LWW5aVivjY Signed-off-by: bigsheeper <yihao.dai@zilliz.com>
|
Thanks, all nine were real. Fixed in High. The page contradicted itself: the transport section claimed a keyless request behaves as before, while the Insert section said Milvus derives a key from the payload. Scoped the first claim to collections with idempotency off, and added a note that two intentionally distinct inserts with identical payloads collapse into one, with the two ways out: an explicit key per insert, or leave the collection property off. Medium. Added a "The global switch was turned off" case and a matching clause in the config table. Replaced the blanket "accepts and ignores" with the Go SDK's client-side rejection on Low. Explained that Two things I left alone deliberately: the pre-existing "Duplicate handling" bullet in |
Adversarial review found no issues requiring changes. This round re-checked the fixes landed in commit Verified:
No new findings survived verification, and no previously raised item is outstanding. |
Re-review of
|
Second review round on this page found two places where the guide contradicted its own model. - The client-support note said a keyless request from an older SDK "behaves as a normal, non-idempotent one" without qualification. That is the same unscoped claim removed from the transport section one commit earlier: on a collection with idempotent insert enabled, a keyless insert still gets an automatic content-derived key. Scope the note the same way, and say SDK support is pending release rather than cite versions that do not exist yet. - Key reuse was described as a complete no-op, which contradicts the per-shard model the page states two sections earlier. A shard the original insert never reached has no record of the key and applies the new rows, so a reused key can leave part of the new batch written while the response describes the original one. Describe that outcome and narrow when the reuse error is actually raised. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Fjo8onCWrFYYkzsECo1DHN Signed-off-by: bigsheeper <yihao.dai@zilliz.com>
|
Agreed on all three. Fixed in High, the client-support note. This was my own regression: fixing the previous round's version-note item reintroduced the very unscoped claim that round had removed from the transport section. Now scoped the same way, so an older SDK's keyless insert on an idempotency-enabled collection is described as still getting an automatic content-derived key. High, key reuse. Confirmed against the server code, not only the doc's model, so this can be treated as verified rather than derived:
The page now describes that outcome and narrows when the error is actually raised. I kept the per-shard behavior as the model rather than flattening it, since it is what lets a retry after a partial failure complete the write. Low, SDK versions. You are right that my previous reply overclaimed: I added a note naming the required API surface, not the versions. Since milvus-io/pymilvus#3784 is still open there is no number to cite, so the note now says support is pending release and that this page will name the minimum versions once they ship. |
Adversarial review of this docs-only PR found one medium and five low documentation issues in the new idempotency guide and its cross-link; no blocking correctness problems in the server-side claims that could be checked. Medium
Low
|
- The Insert Entities cross-link told readers to send an idempotency key without saying idempotent insert is off by default, so following it on a default cluster turns a working insert into a deterministic 1100. - The insert walkthrough covered only the "write completed, response lost" branch, though its own trigger is a client timeout. State that a retry arriving mid-write waits for the original and returns its result or its error, mirroring what the import section already says. - The reuse rule and the insert reuse case disagreed on whether Milvus rejects a reused key. It does not refuse the request: the mismatch error is set after the appends have already gone through, so say that explicitly rather than calling it a rejection. - The window claim implied an outage preserves a key. The window is one object per shard shared by every writer, and eviction is commit triggered, so other writers' traffic can evict it meanwhile. - Both tombstone parameters are Export: false and absent from the shipped milvus.yaml, so note that they must be added by hand. - "The returned jobId always refers to a job that exists" contradicted the cleaned-up-job case; scope it to the registration race. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Fjo8onCWrFYYkzsECo1DHN Signed-off-by: bigsheeper <yihao.dai@zilliz.com>
|
All six addressed in Medium, the Insert Entities cross-link. Agreed, and this is the one I would have wanted caught before merge even though the round is non-blocking: a reader on a default cluster following that bullet gets a deterministic 1100. The bullet now says idempotent insert must be enabled on the collection, that it is off by default, and that sending a key beforehand is rejected. I had told an earlier round I was leaving this bullet alone as "accurate as written". It was accurate and incomplete in a way that misleads, which is worse in a doc than an error the reader can see. Where I diverge: the reuse-rejection contradiction. The suggestion was to scope the general rule to "rejects a reused key only in the narrow shape-mismatch cases". The ordering in resp := streaming.WAL().AppendMessagesWithOptions(...) // writes happen here
if err := resp.UnwrapFirstError(); err != nil { ...; return nil }
it.result.Timestamp = resp.MaxTimeTick()
if it.idempotencyEnabled {
warnOnPartialIdempotentDuplicate(...)
if err := mergeDuplicateInsertResults(it.result, resp); err != nil {
it.result.Status = merr.Status(merr.WrapErrParameterInvalidMsg("idempotency key was reused with a different payload; ..."))
}
}The 1100 is set after the append already succeeded, so nothing is refused. Calling it a rejection would tell a reader that a 1100 means nothing was written, when the rows on the non-duplicate shards are already in the WAL. That is the more dangerous misreading and it compounds the partial-write case fixed in the previous round. The general rule now says Milvus "does not refuse the request", and the insert case adds that the error is reported after the writes have gone through, so it is a warning that the two requests disagree rather than a sign that nothing landed. The other four. Added the in-flight sentence to the insert walkthrough, mirroring the import section. Scoped the window claim: the window is one object per shard shared by every writer and eviction is commit triggered, so other writers' traffic can evict a key during an outage. Marked both tombstone rows as absent from the shipped yaml, which I confirmed in the milvus tree ( |
Adversarial review of the new idempotent-requests guide and its cross-links found seven accuracy issues in the added text, all concentrated in the bulk-import window semantics, the insert window's durability scope, and the configuration table; no earlier-round item is re-raised, since all twelve were resolved in High
Medium
Low
|
Four review rounds pulled this page toward spec completeness: every incomplete sentence grew a qualifier, and a reader who only wants a safe retry ended up reading about per-channel byte budgets, tombstone re-stamping on restart, a length floor on a parameter they will never set, and DDL duplicate-rejection hazards. All of it true, none of it what this page is for. Cut from 2283 words to 986. Kept what a user acts on: both switches and the error if you send a key too early, retry with the same key, what key reuse actually does, the derived-key trap for identical payloads, that the window is bounded by other writes rather than by time, and the two import cases that need a new key. Moved parameter detail to a pointer at the configuration reference, which already documents the interactions better than a table can. Also attribute idempotent insert to the cluster setting plus the collection property in the Insert Entities cross-link, not the property alone, so a reader who follows it does not enable half of it and get error 1100. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Fjo8onCWrFYYkzsECo1DHN Signed-off-by: bigsheeper <yihao.dai@zilliz.com>
|
Thanks. Rather than fix these one by one, I rewrote the page in Why the rewriteFour review rounds each found statements that were incomplete, and each fix added a qualifier. That is the correct standard for a design doc, and applying it to a user guide produced a page where someone who only wants a safe insert retry had to read about per-channel byte budgets, tombstone re-stamping across restarts, a length floor on a parameter they will never set, and DDL duplicate-rejection hazards. Every one of those statements is true. I verified them all. None of them belong on this page. The root problem was that the page was three documents at once: a user guide, a configuration reference, and a semantics spec. The spec parts invited spec-level scrutiny, so each round found more holes and the page grew another layer of hedging. The principle I would ask later reviewers to applyJudge this page by whether a reader can act on it, not by whether every sentence is complete. Concretely:
If a future round finds a removed statement missing, that is expected. A shorter sentence is by construction less complete. Please weigh it against the bar above before asking for it back. What I did keep from this round
What I dropped on purposeThe per-channel versus per-shard durable budget, the 64-byte floor on |
This PR now adds one markdown file and nothing else. The earlier commits also registered the page in the sidebar and linked to it from Insert Entities and Import Data. Those encode decisions that belong to a doc engineer, not to me: which section the page sits in, what it is called in the nav, where it falls in the order, and which existing pages should point at it. Revert all three so the review is about the content. Merged as-is the page has no sidebar entry and is reachable only by direct link. That placement is the remaining work, and the PR description now says so. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Fjo8onCWrFYYkzsECo1DHN Signed-off-by: bigsheeper <yihao.dai@zilliz.com>
This review of the new Medium
Low
|
Rendering the review comment now. Post-verification review of the new idempotent-requests page found one medium-severity snippet regression from the rewrite and two low-severity wording and cross-reference inaccuracies, all introduced by this PR. Medium
Low
|
|
I checked every technical claim on this page against the server code and they all hold: the I also agree with the cut from 2283 to 986 words and would not add the detail back. One Must fix"Two operations honor the key today: insert and bulk import." Insert does not, yet — Worth fixing"Each shard remembers recent keys up to a byte budget." There is also a cap on how many "At most 256 bytes of printable ASCII." 256 is the default of OptionalThe SDK exception note will need updating. The page notes that the Go SDK's |
Draft of a user guide for the idempotency key introduced by milvus-io/milvus#52544 (bulk import) and milvus-io/milvus#50007 (insert).
This PR adds one markdown file and nothing else. Placement and linking are left to a doc engineer. The page is not registered in
menuStructure/en.jsonand nothing links to it, so merged as-is it is reachable only by direct link. Which section it belongs to, what it is called in the nav, and which pages should point at it are decisions I did not want to make unilaterally. Earlier revisions of this PR made them; those commits are reverted.What the page covers
Idempotency-Key, gRPC metadataidempotency-key, with pymilvus, Go SDK and curl examples.jobIdproblem, collection scoping across renames, and the two cases that need a new key.Scope
This is a user guide, deliberately not a specification. It went through four adversarial review rounds that pulled it toward spec completeness, and at 2283 words a reader who only wanted a safe retry was reading about per-channel byte budgets, tombstone re-stamping across restarts, a length floor on a parameter they will never set, and DDL duplicate-rejection hazards. All true, none of it useful here. The page was cut back to 986 words in
5406ad0e.The standard I would ask reviewers to apply: judge it by whether a reader can act on it, not by whether every sentence is complete. Precision that changes no reader decision belongs in the parameter docs and the design doc, which already carry it. The bar for adding something back is a concrete user action that goes wrong without it.
Status of the underlying features
🤖 Generated with Claude Code
https://claude.ai/code/session_01Fjo8onCWrFYYkzsECo1DHN