fix(es): Treat a bulk 409 as an idempotent success in the sync writer - #9113
Merged
yurishkuro merged 5 commits intoJul 31, 2026
Merged
Conversation
Contributor
Author
|
@yurishkuro PTAL |
Me-Priyank
force-pushed
the
fix/es-sync-bulk-conflict-idempotent
branch
from
July 25, 2026 11:39
18c8bf3 to
17752fc
Compare
RFC 0007 §4.7 requires that a 409 returned for a document written with our own deterministic `_id` count as an idempotent success: under op_type: create, which data streams force, a retried span comes back as a version conflict because the byte-identical document is already stored. The async indexer implements this (OnFailure routes 409 to onItemConflict). SyncBulkWriter did not: isTransientStatus lists only 0/429/5xx, so a 409 fell through to the terminal branch and was classified as a poison pill. Under the default poison_pill_handling: fail that fails the batch, holds the Kafka offset, and head-of-line blocks the partition on every redelivery; under drop it discards already-durable documents and reports them as poison. classify now counts a 409 separately, as neither transient nor terminal, so it contributes to the chunk's durable total and is logged at debug like the async peer. This is safe because op_type: create is only ever sent by the span writer, whose `_id` is a content hash of the document, so a conflict means the stored document is the one being written. classify returns a bulkOutcome struct rather than a fourth result because revive's function-result-limit caps a function at three return values. Tests: TestSyncBulkWriter_ConflictIsIdempotent is the sync peer of TestBulkIndexerConflictIsIdempotent. TestSyncBulkWriter_ItemErrorPropagates kept its purpose (item-level error propagation, RFC 0007 M2) but now uses a 400 mapper_parsing_exception, since a 409 is no longer a rejection. The live integration assertion likewise now proves the 409 is idempotent, and a mapping conflict covers terminal propagation, which is the injection RFC 0007 M5 prescribes. This makes the M3 cross-reference in TestElasticsearchStorage_ WriteIdempotency accurate: the live 409 now does demonstrate a benign idempotent write. Part of jaegertracing#8476 Signed-off-by: Me Priyank <pvonlyone29@gmail.com>
Me-Priyank
force-pushed
the
fix/es-sync-bulk-conflict-idempotent
branch
from
July 25, 2026 12:12
17752fc to
09d6ca0
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9113 +/- ##
=======================================
Coverage 97.84% 97.85%
=======================================
Files 377 377
Lines 19812 19821 +9
=======================================
+ Hits 19386 19395 +9
Misses 289 289
Partials 137 137
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
yurishkuro
approved these changes
Jul 31, 2026
yurishkuro
added
the
changelog:experimental
2 tasks
CI Summary ReportMetrics ComparisonView changed metricsFor label-level diff details, open the CI run and expand the "Compare metrics and generate summary" step logs. metrics_snapshot_kafka_v2 — ⬇️ download diff
Code Coverage✅ Coverage 98.4% (baseline 98.4%) ➡️ View CI run | View publish logs |
Member
|
🔥 |
github-actions
Bot
added
the
waiting-for-author
yurishkuro
added a commit
to ysh-bot/jaeger
that referenced
this pull request
Jul 31, 2026
The connector's primary job is the synchronous storage write; dead-lettering is an exceptional tap on its output. Naming it jaeger_storage_dead_letter made the main pipeline read backwards — `exporters: [jaeger_storage_dead_letter]` naively scans as "export to a dead letter" when it actually writes to storage. Rename the type (and package) to jaeger_storage_writer so the exporter slot reads as "write to storage"; the dead-letter role is carried by the output pipeline it feeds (traces/dead_letter), not the component name. Also rebased onto main to fold in jaegertracing#9113: classify() now returns the 409-as- idempotent-conflict tally alongside the []RejectedItem terminal set the connector needs, so a retried-and-already-durable span is never mis-routed to dead-letter. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Yuri Shkuro <github@ysh.us>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which problem is this PR solving?
Description of the changes
409returned for a document written with Jaeger's own deterministic_idcount as an idempotent success: underop_type: create, which data streams force, a retried span comes back as a version conflict because the byte-identical document is already stored.OnFailureroutes409toonItemConflict).SyncBulkWriterdid not:isTransientStatuslists only0 / 429 / 5xx, so a409fell through to the terminal branch and was classified as a poison pill. Under the defaultpoison_pill_handling: failthat fails the batch, holds the Kafka offset, and head-of-line blocks the partition on every redelivery; underdropit discards already-durable documents and reports them as poison.classifynow counts a409separately, as neither transient nor terminal, so it contributes to the chunk's durable total and is logged at debug like the async peer.op_type: createis only ever sent by the span writer (core/writer.go, the sole non-test site that setsOpType), whose_idis a content hash of the document, so a conflict means the stored document is the one being written. Service, sampling and dependency writes leaveOpTypeempty, so they useindexand upsert.classifyreturns a smallbulkOutcomestruct rather than a fourth result because revive'sfunction-result-limitcaps a function at three return values. That accounts for theterminal→out.terminalchurn insendChunk; there is no behaviour change in that rename.Scope note: this is latent rather than a regression in a shipped path.
DataStreamRotationis the only rotation returningWriteOpCreate, andindices.<type>.rotation.data_streamis still rejected at startup, so no deployed pipeline reaches it today. It becomes live when data-stream rotation is wired, so it lands ahead of that work.How was this change tested?
TestSyncBulkWriter_ConflictIsIdempotent(new) is the sync peer ofTestBulkIndexerConflictIsIdempotent: a409 version_conflict_engine_exceptiononop_type: createreturns no error, counts inbulk_index.inserts, logs no error, and logs the conflict at debug.TestSyncBulkWriter_ItemErrorPropagateskeeps its original purpose (item-level error propagation, RFC 0007 M2) but now uses a400 mapper_parsing_exception, since a409is no longer a rejection. No existing test was deleted.TestElasticsearchStorage_SyncBulkWriternow proves the409is idempotent against a real backend (re-creating an existing_idreturns no error and adds no duplicate), and a mapping conflict covers terminal propagation, which is the injection RFC 0007 M5 prescribes.TestElasticsearchStorage_WriteIdempotencyaccurate: the live409now does demonstrate a benign idempotent write, which is what that comment already claimed.sync_bulk.gopatch coverage is 100% (123/123 statements).golangci-lintreports 0 issues,gofmt/gofumptare clean, andgo build ./...passes.STORAGE=elasticsearchagainst a live backend, so they are covered here by compilation andgo vetonly and rely on the ES 7-9 / OS 1-3 CI matrix to exercise them.Checklist
make lint testAI Usage in this PR (choose one)