deb: rename published indexes concurrently - #1633
Open
barnumbirr wants to merge 1 commit into
Open
Conversation
RenameFiles walks renameMap one entry at a time. Every rename is a round trip, and on an S3 backend RenameFile is a CopyObject followed by a DeleteObject, so a publish that rewrites a dozen indexes spends most of its wall clock waiting on them in sequence. Bounded at four in flight, matching the default used for concurrent package uploads. The first error is returned rather than the last, and the remaining renames still run, so a failure does not leave part of the set staged behind a half-finished switch. Measured against MinIO behind a proxy adding 50ms each way, publishing a repository whose Contents is dominated by one package carrying 19,500 files: publish update drops from 2703ms to 1624ms median, n=5 either side, a 40% cut. The published object set and its sizes are identical before and after. RenameFiles had no test coverage. It has four cases now: every entry renamed, the concurrency limit respected, a failure reported without abandoning the rest, and an empty rename map.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1633 +/- ##
==========================================
- Coverage 77.37% 77.10% -0.28%
==========================================
Files 165 165
Lines 15747 15764 +17
==========================================
- Hits 12185 12155 -30
- Misses 2356 2408 +52
+ Partials 1206 1201 -5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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
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.
Fixes #1632
Requirements
All new code should be covered with tests, documentation should be updated. CI should pass.
Description of the Change
RenameFileswalksrenameMapone entry at a time. Every rename is a round trip, and on an S3 backendRenameFileis aCopyObjectfollowed by aRemove, so a publish that rewrites a dozen indexes spends most of its wall clock waiting on them in sequence. #1632 has the traces and the background.This runs them concurrently, bounded at four in flight to match the default used for concurrent package uploads in #1616. The first error is returned rather than the last, and the remaining renames still run, so a failure does not leave part of the set staged behind a half finished switch.
Nothing else changes: the same renames happen, the published object set and its sizes are identical before and after, and the
.tmpstaging that makes every index and the signature switch together is untouched.Measurements
Against MinIO behind a proxy adding 50ms each way, publishing a repository whose
Contentsis dominated by one package carrying 19,500 files.publish update, n=5 each:A 40% cut. For reference, removing the staging entirely gets to 1240ms, so this captures about 75% of what the rename phase could ever give back. The rest is not worth it: writing indexes straight to their final names means a failure part way through the upload leaves the archive with some indexes new and some old and no way back, which is exactly what the
.tmpset prevents.Four in flight is a constant rather than a config key, since nothing yet asks for a different value. Happy to make it configurable the way #1616 does if you would rather the two matched.
Notes for review
FinalizeAllis serial in the same way, so the upload phase has similar headroom, but it calls the signer and I have not looked at whether that is safe to run concurrently. Batching theRemovecalls into oneDeleteObjectswas also considered; it can only reach part of what survives this change and would need a change to thePublishedStorageinterface that every backend implements.The test file is named
index_files_rename_test.gorather thanindex_files_test.godeliberately: #1626 adds a file by the latter name, and this way the two can land in either order without an add/add conflict.Checklist
AUTHORSRenameFileshad no coverage at all before this. It now has four cases: every entry renamed, the concurrency limit respected, a failure reported without abandoning the rest, and an empty rename map. Locallygo test ./deb/passes withRenameFilesat 100%,golangci-lintv2.12.2 reports 0 issues, andgofmt,go vetandgo build ./...are clean.