-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
AWS S3 Upload Large Files #15386
AWS S3 Upload Large Files #15386
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 3 Skipped Deployments
|
WalkthroughThis pull request introduces updates to multiple AWS S3-related components, primarily focusing on improving file upload functionality and version increments. The most significant change is in the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (4)
components/aws/sources/s3-deleted-file/s3-deleted-file.mjs (1)
Line range hint
13-18
: Consider enhancing the property documentation.While the current description is clear, it would be helpful to add examples of when users might want to enable/disable this option, and explain the difference between delete markers (used in versioned buckets) vs permanent deletions.
components/aws/sources/s3-restored-file/s3-restored-file.mjs (2)
Line range hint
13-18
: Consider enhancing the property documentation.While the current description is clear, it would be helpful to add:
- Examples of when users might want to track restore initiation
- Explanation of the S3 Glacier restore lifecycle
- Expected time gaps between initiation and completion events
Line range hint
28-37
: Update summary message to reflect restore initiation events.When
detectRestoreInitiation
is enabled, the summary message should distinguish between initiation and completion events.Consider updating the
generateMeta
method:generateMeta(data) { const { "x-amz-request-id": id } = data.responseElements; const { key } = data.s3.object; const { eventTime: isoTimestamp } = data; + const isInitiation = data.eventName === "s3:ObjectRestore:Post"; return { id, - summary: `Restored file: '${key}'`, + summary: isInitiation + ? `Initiated restore for file: '${key}'` + : `Restored file: '${key}'`, ts: Date.parse(isoTimestamp), }; };components/aws/common/common-s3.mjs (1)
84-88
: Consider adding error handling and progress tracking.While the implementation is correct, consider these enhancements for better reliability and user experience:
- Add error handling
- Implement upload progress tracking
Here's a suggested implementation:
async uploadFile(params) { const parallelUploads3 = new Upload({ client: await this._clientS3(), params, + queueSize: 4, // optional: number of concurrent uploads }); + // Optional: Track progress + parallelUploads3.on("httpUploadProgress", (progress) => { + console.log(progress.loaded, progress.total); + }); - await parallelUploads3.done(); + try { + await parallelUploads3.done(); + } catch (err) { + // Handle specific errors + if (err.name === "AbortError") { + throw new Error("Upload aborted by user"); + } + throw new Error(`Upload failed: ${err.message}`); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yaml
is excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (13)
components/aws/actions/s3-download-file-to-tmp/s3-download-file-to-tmp.mjs
(1 hunks)components/aws/actions/s3-generate-presigned-url/s3-generate-presigned-url.mjs
(1 hunks)components/aws/actions/s3-stream-file/s3-stream-file.mjs
(1 hunks)components/aws/actions/s3-upload-file-tmp/s3-upload-file-tmp.mjs
(1 hunks)components/aws/actions/s3-upload-file-url/s3-upload-file-url.mjs
(1 hunks)components/aws/actions/s3-upload-file/s3-upload-file.mjs
(1 hunks)components/aws/common/common-s3.mjs
(2 hunks)components/aws/package.json
(2 hunks)components/aws/sources/new-emails-sent-to-ses-catch-all-domain/new-emails-sent-to-ses-catch-all-domain.mjs
(1 hunks)components/aws/sources/s3-deleted-file/s3-deleted-file.mjs
(1 hunks)components/aws/sources/s3-new-event/s3-new-event.mjs
(1 hunks)components/aws/sources/s3-new-file/s3-new-file.mjs
(1 hunks)components/aws/sources/s3-restored-file/s3-restored-file.mjs
(1 hunks)
✅ Files skipped from review due to trivial changes (9)
- components/aws/actions/s3-stream-file/s3-stream-file.mjs
- components/aws/actions/s3-download-file-to-tmp/s3-download-file-to-tmp.mjs
- components/aws/actions/s3-upload-file-url/s3-upload-file-url.mjs
- components/aws/actions/s3-upload-file/s3-upload-file.mjs
- components/aws/sources/new-emails-sent-to-ses-catch-all-domain/new-emails-sent-to-ses-catch-all-domain.mjs
- components/aws/sources/s3-new-event/s3-new-event.mjs
- components/aws/actions/s3-upload-file-tmp/s3-upload-file-tmp.mjs
- components/aws/sources/s3-new-file/s3-new-file.mjs
- components/aws/actions/s3-generate-presigned-url/s3-generate-presigned-url.mjs
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Publish TypeScript components
- GitHub Check: Verify TypeScript components
🔇 Additional comments (7)
components/aws/sources/s3-deleted-file/s3-deleted-file.mjs (2)
9-9
: LGTM! Version bump is appropriate.The version increment from 0.1.2 to 0.1.3 aligns with the addition of the new
ignoreDeleteMarkers
feature.
Line range hint
22-27
: LGTM! Event filtering is implemented correctly.The conditional event type selection based on
ignoreDeleteMarkers
is well implemented:
s3:ObjectRemoved:Delete
for permanent deletionss3:ObjectRemoved:*
for all deletion eventscomponents/aws/sources/s3-restored-file/s3-restored-file.mjs (2)
9-9
: LGTM! Version bump is appropriate.The version increment from 0.1.2 to 0.1.3 aligns with the addition of the new
detectRestoreInitiation
feature.
Line range hint
22-27
: LGTM! Event filtering is implemented correctly.The conditional event type selection based on
detectRestoreInitiation
is well implemented:
s3:ObjectRestore:*
for both initiation and completions3:ObjectRestore:Completed
for completion onlycomponents/aws/common/common-s3.mjs (1)
11-11
: LGTM! Good choice of library for large file uploads.The addition of
@aws-sdk/lib-storage
is appropriate as it provides built-in support for multipart uploads and better handling of large files.components/aws/package.json (2)
3-3
: LGTM! Version bump follows semantic versioning.The increment from 0.7.1 to 0.7.2 is appropriate for this enhancement.
28-28
: Consider aligning AWS SDK dependency versions.The new dependency
@aws-sdk/lib-storage
is on version ^3.734.0 while other AWS SDK dependencies are on older versions (^3.58.0 to ^3.731.0). Consider aligning all AWS SDK dependency versions for better compatibility and maintenance.Let's check for any compatibility issues between these versions:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @michelle0927 lgtm! Ready for QA!
Resolves #15382
Summary by CodeRabbit
New Features
ignoreDeleteMarkers
option for S3 deleted file eventsdetectRestoreInitiation
option for S3 restored file eventsImprovements
@aws-sdk/lib-storage
Version Updates