fix(s3): re-land the same-id concurrency guard + raw coalescing dropped from #1411 - #1433
Open
angela-helios wants to merge 2 commits into
Open
angela-helios wants to merge 2 commits into
angela-helios wants to merge 2 commits into
Conversation
…ess) The concurrent ingest path races two entries that target the same resource id: their conditional writes interleave to a non-deterministic final state, where the serial loop resolves them last-write-wins in file order. A batch with any id collision therefore falls back to the serial path; a bulk file usually carries distinct resources, so the common case still parallelizes. Entries with no client id are server-assigned a unique one and never collide. Test: two entries for the same id, second content wins. Flagged by the #945 perf measurement (5.7x at N=16, latency-bound, 7 PUTs/entry).
The per-entry raw-line PUT was one of the 7 PUTs/resource the #1429 measurement found the ingest is bound by, and the raw archive has zero production readers — it is a write-only auditable copy of the input. Write it once per batch instead: persist_raw_batch archives the whole chunk's NDJSON in a single object, keyed by the batch's first line number (submit_raw_batch_key) so successive chunks of one file never collide, and still discriminated by file_url like the receipts (#457). Written upfront, before any entry processing, so a failure aborts before anything is stored. Drops the raw PUT from 1/entry to 1/batch — 7->6 PUTs per resource, and one of the follow-ups the #1429 phase breakdown pointed at (raw + change + receipt are the coalescible three; change and receipt have readers and come next). Tests: one raw object per batch holding every line, and the #457 two-file archive discrimination updated to the batch key.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
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
1 participant
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.
Why
#1411 was merged at its first commit (
553030d8b, bare bounded concurrency) before its follow-up commits landed on the branch, so two changes that were part of that work never reached main:fix(s3): keep same-id entries in a batch serial— a correctness fix. Without it, the concurrent ingest path races two entries in one batch that target the same resource id: their conditional writes interleave to a non-deterministic result, where the serial loop resolves them last-write-wins in file order. main currently has the concurrent path without this guard. The guard falls a batch with any id collision back to serial; distinct-id batches (the common case) still parallelize.perf(s3): coalesce the raw NDJSON archive to one object per batch— 7→6 PUTs/resource. The raw archive has zero production readers, so it's written once per batch (persist_raw_batch, keyed by first line,file_url-discriminated per bulk-submit: multi-file manifests collide in bulk_entry_results — the PK has no file discriminator, so every file after the first fails at its first line #457) instead of per entry.Both were validated on MinIO by the #945/#1429 measurement (correctness: 0 errors / 0 lease-lost, rollback OK, same-id → last write wins over 5 rounds; perf: raw coalescing +14–15% A/B, objects/resource 7.0→6.01) — see #1429 — but that measurement ran against the full branch head (
1434c83e1), which is not what merged.What
Cherry-picks of
61665ea60and1434c83e1onto current main (they were built on553030d8b, which is in main, so they apply cleanly). No new work.Test
s3::testsbulk-submit suite green (27): same-id batch stays serial (second write wins), 20-entry concurrent batch stays ordered and fully written, raw archive is one object per batch, #457 two-file discrimination on the batch key. fmt + clippy clean under the CI allow-flags.