feat: Stream S3 organisation exports #6612
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 3 Skipped Deployments
|
Docker builds report
|
github-actions
bot
added
api
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6612 +/- ##
==========================================
+ Coverage 98.20% 98.24% +0.04%
==========================================
Files 1298 1301 +3
Lines 47108 47262 +154
==========================================
+ Hits 46262 46434 +172
+ Misses 846 828 -18 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
github-actions
bot
added
feature
khvn26
force-pushed
the
feat/streaming-s3-export
branch
from
9fdb083 to
502c108
Compare
github-actions
bot
added
feature
emyller
left a comment
emyller
left a comment
There was a problem hiding this comment.
I'm interested in experimenting this right now.
When is the S3 object URL made available to the requester?
@emyller The current process is to retrieve it manually. |
matthewelwell
left a comment
matthewelwell
left a comment
There was a problem hiding this comment.
Looks good on the whole - one question on the testing approach.
|
|
||
| @pytest.fixture | ||
| def s3_client() -> MagicMock: | ||
| client = MagicMock() |
There was a problem hiding this comment.
Couldn't we use moto here to test this more realistically?
There was a problem hiding this comment.
Done in 8f8b385 (which includes a bugfix thanks to moto usage!)
khvn26
force-pushed
the
feat/streaming-s3-export
branch
from
02c43d4 to
8f8b385
Compare
github-actions
bot
added
feature
emyller
left a comment
emyller
left a comment
There was a problem hiding this comment.
A couple non-blocking comments. Otherwise LGTM!
| # When | ||
| try: | ||
| with S3MultipartUploadWriter(s3_client, bucket_name, key) as writer: | ||
| writer.write(b"some data") | ||
| raise ValueError("test error") | ||
| except ValueError: | ||
| pass |
There was a problem hiding this comment.
Note: I'd see value in wrapping this under pytest.raises, as we're also validating S3MultipartUploadWriter re-raises.
| """ | ||
|
|
||
| # S3 multipart upload minimum part size is 5MB (except for the last part) | ||
| MIN_PART_SIZE = 5 * 1024 * 1024 |
There was a problem hiding this comment.
Is MIN_PART_SIZE = every chunk size?
IMO this could be a greater value that helps reducing network round trips without compromising the app's memory, e.g. 64MB:
- If export < 100MB, two chunks are good!
- Otherwise, a recent realistic example could go from 250 chunks to 20.
Also worth noting: our Customer Success KB recommends an ECS task with a lot of memory.
- Reminder: update it to spend less resources.
Thanks for submitting a PR! Please check the boxes below:
docs/if required so people know about the feature.Changes
Contributes to #6610.
In this PR, we avoid OOM worker terminations when attempting to export beefy organisations via the sales dashboard.
We assume that using generators and multipart S3 uploads should significantly reduce the memory footprint. The tradeoff is that the exports should take slightly longer.
How did you test this code?
All the new code and changes to existing code are covered with tests. This will be tested manually on staging.