Support for concurrent S3 part uploads - #2510
Conversation
This will be send to Wings when generating new backup
|
All contributors have signed the CLA ✍️ ✅ |
📝 WalkthroughWalkthroughThe change adds a default and environment override for backup upload concurrency. ChangesBackup upload concurrency
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/Extensions/BackupAdapter/Schemas/S3BackupSchema.php (1)
149-152: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winUpdate the
getUploadPartsreturn shape.The
@returnannotation at Line 103 declares onlypartsandpart_size, but the method now returnsmax_concurrent_uploads. Update the annotation so static analysis and callers see the complete response contract.Proposed annotation update
- /** `@return` array{parts: string[], part_size: int} */ + /** `@return` array{parts: string[], part_size: int, max_concurrent_uploads: int} */🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Extensions/BackupAdapter/Schemas/S3BackupSchema.php` around lines 149 - 152, Update the `@return` annotation for getUploadParts to include the max_concurrent_uploads field alongside parts and part_size, matching the complete array returned by the method.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/Extensions/BackupAdapter/Schemas/S3BackupSchema.php`:
- Around line 144-152: Validate $maxConcurrentUploads in the S3 backup schema
before returning it, treating zero, negative values, and non-numeric
configuration values as invalid and falling back to
BackupRemoteUploadController::MAX_CONCURRENT_UPLOADS; also update the PHPDoc
return shape to declare max_concurrent_uploads.
---
Nitpick comments:
In `@app/Extensions/BackupAdapter/Schemas/S3BackupSchema.php`:
- Around line 149-152: Update the `@return` annotation for getUploadParts to
include the max_concurrent_uploads field alongside parts and part_size, matching
the complete array returned by the method.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 248a4778-cd96-4cec-8e8c-c6f986a472d6
📒 Files selected for processing (3)
app/Extensions/BackupAdapter/Schemas/S3BackupSchema.phpapp/Http/Controllers/Api/Remote/Backups/BackupRemoteUploadController.phpconfig/backups.php
Currently, when uploading an S3 backup, wings can only upload one part at the time.
Following PR #2504 / issue #2491, S3 backups have been made more reliable and the network overhead has been reduced.
However, a single TCP connection is limited by congestion control (slow start, window scaling) and rarely saturates a fast link on its own.
S3 supports concurrent uploads. This allows for faster uploads by making full use of the available bandwidth.
This PR simply aims to:
BACKUP_MAX_CONCURRENT_UPLOADSparameter, set to10by defaultThe default of 10 is based on benchmarks in the Wings PR: most of the throughput gain is already reached at moderate concurrency, and 10 scales well to large backups (hundreds of parts) while keeping connection pressure on the S3 endpoint reasonable.
If Wings receives no value or an invalid one (e.g. from an older Panel), it falls back to sequential uploads.
The new S3 asynchronous multipart upload logic is located in Wings, in PR pelican/wings#205