Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

perf(artifacts): pipeline the artifact file uploader #7586

Merged
merged 2 commits into from May 21, 2024

Conversation

moredatarequired
Copy link
Contributor

@moredatarequired moredatarequired commented May 7, 2024

Description

Rewrite of ArtifactSaver.uploadFiles to use buffered channels and switch between available pending tasks

This is an alternative to #7274, and should be considered to supersede it. It's dramatically simpler, has similar (though slightly lower) performance, and handles retries better.

Key differences:

  1. We don't impose an arbitrary minimum time (previously 1 hour) before retrying failed uploads
  2. We bound the number of outstanding goroutines to limit maximum memory consumption
  3. Overall this version is 20-25% faster for artifacts with many small files

Minor differences:
Differences:

  1. I hope that this version is simplified and a little easier to follow, though YMMV
  2. We use a smaller batch size (1000); this decreases stuttering and improves performance especially for 2k-10k file artifacts
  3. We don't pause after requesting a batch of URLs, and can immediately start scheduling uploads made available from a previous batch
  4. We process upload responses as they come in, rather than a tick-tock pattern where we alternate between scheduling them and processing them in ~10k batches
  5. By selecting among all available tasks we are able to terminate earlier in case of fatal errors; this will also allow us to more easily integrate progress reporting with a time.Ticker channel in the future

Data flow:
Assemble batches
While not done:

  1. If there are 1000 or fewer outstanding uploads, request upload URLs for a batch (in the background)
  2. Select first available
    • Error to handle
    • File to schedule for upload
    • Upload result to check

If files fail to upload, collect them to retry. Retry all failed uploads by rerunning the above as long as each time we succeed for at least half the files.

  • I updated CHANGELOG.md, or it's not applicable

Testing

Very extensively tested for performance (on extremely large artifacts).

Relies somewhat heavily on existing integration tests in CI for correctness.

Copy link

codecov bot commented May 7, 2024

Codecov Report

Attention: Patch coverage is 77.66990% with 23 lines in your changes are missing coverage. Please review.

Project coverage is 75.85%. Comparing base (849d919) to head (ea5e457).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #7586      +/-   ##
==========================================
+ Coverage   74.26%   75.85%   +1.58%     
==========================================
  Files         500      500              
  Lines       55732    54153    -1579     
==========================================
- Hits        41392    41080     -312     
+ Misses      13932    12663    -1269     
- Partials      408      410       +2     
Flag Coverage Δ
func 41.37% <ø> (+0.33%) ⬆️
system 63.51% <76.69%> (+0.03%) ⬆️
unit 56.29% <0.97%> (+0.93%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...ore/internal/filetransfer/file_transfer_manager.go 85.13% <100.00%> (+18.81%) ⬆️
core/pkg/server/sender.go 71.65% <100.00%> (+9.01%) ⬆️
core/pkg/artifacts/saver.go 75.08% <76.76%> (+9.93%) ⬆️

... and 102 files with indirect coverage changes

@moredatarequired moredatarequired force-pushed the pipeline-artifact-saver branch 3 times, most recently from cb204c5 to 4b45057 Compare May 7, 2024 17:21
@moredatarequired moredatarequired marked this pull request as ready for review May 7, 2024 17:22
@moredatarequired moredatarequired requested a review from a team as a code owner May 7, 2024 17:22
core/pkg/artifacts/saver.go Outdated Show resolved Hide resolved
core/pkg/artifacts/saver.go Outdated Show resolved Hide resolved
core/pkg/artifacts/saver.go Outdated Show resolved Hide resolved
core/pkg/artifacts/saver.go Outdated Show resolved Hide resolved
core/pkg/artifacts/saver.go Show resolved Hide resolved
core/pkg/artifacts/saver.go Show resolved Hide resolved
core/pkg/artifacts/saver.go Outdated Show resolved Hide resolved
core/pkg/artifacts/saver.go Show resolved Hide resolved
core/pkg/artifacts/saver.go Outdated Show resolved Hide resolved
@moredatarequired moredatarequired requested a review from a team as a code owner May 14, 2024 20:45
@moredatarequired moredatarequired force-pushed the pipeline-artifact-saver branch 2 times, most recently from cac9d37 to 80e086f Compare May 15, 2024 00:30
@moredatarequired moredatarequired merged commit d9a1f69 into main May 21, 2024
39 checks passed
@moredatarequired moredatarequired deleted the pipeline-artifact-saver branch May 21, 2024 22:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants