Repository navigation
fix(search): report rejected documents when indexing to OpenSearch - #3142
Conversation
ec1451b to
28e148c
Compare
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 6 |
🟢 Coverage 72.41% diff coverage · +0.08% coverage variation
Metric Results Coverage variation ✅ +0.08% coverage variation (-1.00%) Diff coverage ✅ 72.41% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (b3f16bd) 92640 25432 27.45% Head commit (ff48287) 92661 (+21) 25510 (+78) 27.53% (+0.08%) Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch:
<coverage of head commit> - <coverage of common ancestor commit>Diff coverage details
Coverable lines Covered lines Diff coverage Pull request (#3142) 29 21 72.41% Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified:
<covered lines added or modified>/<coverable lines added or modified> * 100%
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
26a44bb to
9beb00a
Compare
| return fmt.Errorf("failed to delete by query: %w", err) | ||
| case len(resp.Failures) != 0: | ||
| return fmt.Errorf("failed to delete by query, failures: %v", resp.Failures) | ||
| return fmt.Errorf("failed to delete by query, failures: %s", resp.Failures) |
There was a problem hiding this comment.
resp.Failures is a []json.RawMessage, and json.RawMessage is a []byte without a String() method, so %v rendered the failures as byte arrays:
[[123 34 105 110 100 101 120 34 58 34 111 112 101 110 ...]]
%s prints what the API actually returned:
[{"index":"opencloud-demo-dbq","id":"1$1!3","cause":{"type":"cluster_block_exception","reason":"index [opencloud-demo-dbq] blocked by: [FORBIDDEN/8/index write (api)];"},"status":403}]
87dd68a to
3f099cf
Compare
3f099cf to
0cd6e84
Compare
0cd6e84 to
e55e291
Compare
e55e291 to
da8d490
Compare
| return fmt.Errorf("failed to marshal bulk failures: %w", err) | ||
| } | ||
|
|
||
| return fmt.Errorf("failed to execute bulk operations, failures: %s", failures) |
There was a problem hiding this comment.
This makes the loop return at line 281, so every operation queued after this bulk gets skipped. Is that what we want? I'd rather collect the errors and return them joined at the end. Same for the operation errors at line 284 actually, which bail out the same way.
If we keep going after a failed bulk, bulkOperations needs to be reset on the error path too, or the next flush resends it.
There was a problem hiding this comment.
Good point, before the PR a rejected bulk was silently skipped, with it the rest of the batch was dropped. Now the errors are collected and joined, bulkOperations is reset on every path, and a spec pins that a Delete and a second Upsert after a rejected bulk still go through (ff48287).
Batch.Pushdiscarded the bulk response. The bulk API answers 200 even when it rejects individual items, so a rejected document went missing from the index without a trace.Purgealready checksresp.FailuresforDeleteByQuery, this does the same forBulk.A failed bulk or operation no longer stops the batch, the remaining operations are independent of it.
Pushruns them all and returns the errors joined. Before, a rejected item was skipped silently and a failed operation dropped the rest of the batch.While at it: those
DeleteByQueryfailures were printed as byte arrays,resp.Failuresis a[]json.RawMessage.This only makes the loss visible, it does not prevent it. The event is acked either way and nothing retries.