Report a bulk update which failed because the document is missing (#4926) - #4942
Report a bulk update which failed because the document is missing (#4926)#4942batrived wants to merge 2 commits into
Conversation
…nusGraph#4926) pairErrorsWithSubmittedMutation removed every bulk item whose status is 404 from the failure list, whatever operation produced it. That is correct for a delete, because deleting an absent document leaves the index in the state the deletion asked for. An update which returns 404 is a document_missing_exception: the write did not happen. Changing the value of a SINGLE cardinality indexed property produces a deletion of the old value and an addition of the new one against the same document, with isNew false. Both become update operations, and because the mutation has deletions, mutate() withholds the upsert document. If the Elasticsearch document is absent, both items return 404, both were discarded, and the property was never indexed. Every later update to that element took the same path, so the document was never recreated. The element stayed invisible to the mixed index while present in the storage backend, and nothing reported it: mutate returned normally, so even the mutate.exceptions metric stayed at zero. Retain the request type on RequestBytes, which already reads it to decide on the retry_on_conflict parameter but did not keep it, and limit the exemption to delete items. Signed-off-by: Balmukund Trivedi <btrivedipublic@gmail.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
eba16d7 to
2e97a6f
Compare
… delete The previous commit exempted only RequestType.DELETE from the rule that a 404 in a bulk item is a lost write. mutate() sends a field deletion as an UPDATE which runs the parameterized deletion script, so that exemption is too narrow: a document with no fields left to delete is already the state the mutation asked for, and reporting it fails index maintenance which removes stale records and any transaction which deletes a field of a document another transaction removed. Carry the distinction on the mutation instead of reading it off the request type. A whole document deletion and a field deletion both only take content out of the index. An addition is what a 404 loses, and mutate() withholds the upsert from the addition once the mutation also has deletions, which is the case the previous commit set out to report. Signed-off-by: Balmukund Trivedi <btrivedipublic@gmail.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2e97a6f to
3053b67
Compare
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Fixes bulk indexing error reporting so that 404 responses are only ignored for mutations that only remove content (e.g., delete / field-deletion), while 404 for additions/updates (i.e., document_missing_exception) is surfaced as a failure to prevent silent no-ops.
Changes:
- Track whether each bulk item “removes content only” and use it when interpreting bulk item failures.
- Narrow the “ignore 404” behavior to content-removal mutations rather than all operations.
- Add unit coverage for mixed bulk batches and the delete-vs-update 404 distinction.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| janusgraph-es/src/main/java/org/janusgraph/diskstorage/es/rest/RestElasticSearchClient.java | Retains mutation intent per bulk item and updates bulk error pairing to only exempt 404s for removal-only operations. |
| janusgraph-es/src/main/java/org/janusgraph/diskstorage/es/ElasticSearchMutation.java | Adds removesContentOnly metadata and introduces a factory for field-deletion updates. |
| janusgraph-es/src/main/java/org/janusgraph/diskstorage/es/ElasticSearchIndex.java | Switches field-deletion script updates to use the new “removes content only” mutation factory. |
| janusgraph-es/src/test/java/org/janusgraph/diskstorage/es/rest/RestClientBulkItemStatusTest.java | Adds tests validating 404 handling for deletes, field deletions, and updates in bulk responses. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| private static boolean isAbsentDocumentRemoval(final RestBulkResponse.RestBulkItemResponse item, | ||
| final RequestBytes submittedItem) { | ||
| return item.getStatus() == HttpStatus.SC_NOT_FOUND && submittedItem.removesContentOnly; | ||
| } | ||
|
|
| @@ -474,13 +477,23 @@ private List<Triplet<Object, Integer, RequestBytes>> pairErrorsWithSubmittedMuta | |||
| throw new IllegalStateException("There should only be a single item per bulk reponse item entry"); | |||
Fixes #4926.
pairErrorsWithSubmittedMutationremoved every bulk item whose status is 404 from the failure list, whatever operation produced it. That is right for adelete— deleting an absent document leaves the index in the state the deletion asked for — but anupdatereturning 404 is adocument_missing_exception, and the write did not happen.Combined with
mutate()withholding the upsert whenever a mutation has both deletions and additions, changing aSINGLEcardinality indexed property on an element whose document is absent silently no-ops, permanently, with no exception, no log line and no metric.mutatereturns normally, so evenindexProvider.<name>.mutate.exceptionsstays at zero.Change
This implements option (1) from the issue.
RequestBytesalready readsrequest.getRequestType()to decide on theretry_on_conflictparameter but did not keep it, so retaining it makes the operation available where the failure is interpreted. The exemption is then limited todeleteitems.Why I did not also do option (2)
Option (2) — always supplying the upsert — would make the 404 impossible rather than merely visible, and I started there. But
mutation.getAdditions()for aSINGLEcardinality value change holds only the changed property, not every indexed property of the element. Upserting from it would create a document containing just that one field, so the element becomes findable by the changed property while still missing the others. That trades a detectable failure for a silently partial document, which I think is the worse of the two.Making the 404 impossible properly means upserting a document built from the element's full indexed state, which is what
restore()already does, and that is a larger change than this fix. Surfacing the error keeps the existing repair paths — transaction log recovery and reindex — in charge of rebuilding the document. Happy to follow up with the upsert version if you would rather have it, or to reconsider if you think the partial document is acceptable.Behaviour change to be aware of
A mutation which previously returned normally now raises a
PermanentBackendException. For a user whose index has already diverged, commits touching those elements will start failing visibly where they used to pass. That is the intent — the alternative is that the divergence stays invisible — but it is a real change in what a running deployment sees, so it may deserve a changelog note if you want one; say so and I will add it.Testing
RestClientBulkItemStatusTestcovers the delete exemption, an update 404, an index 404, the deletion-plus-addition shape thatmutate()actually produces for aSINGLEcardinality value change, a mixed batch where only the genuinely failed item is reported, and a batch where an exempt delete and a reported update arrive together.I confirmed four of these fail against
masterwith "Expected java.io.IOException to be thrown, but nothing was thrown", which is the silent discard. The existingRestClientRetryTeststill passes, so the retry path that also consumes this method is unaffected. Docker was unavailable to me, so I could not run the container-backed Elasticsearch suites locally.Note on overlap
This touches
pairErrorsWithSubmittedMutationandRequestBytes, which my PR #4935 for #4925 also modifies nearby, so whichever merges second will need a small rebase.For all changes:
master)?For code changes: