-
Notifications
You must be signed in to change notification settings - Fork 868
Add UploadIniaited, Completed, and Failed event progress trackers to SimpleUploadCommand #4039
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
Add UploadIniaited, Completed, and Failed event progress trackers to SimpleUploadCommand #4039
Conversation
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.
Pull Request Overview
This PR implements simple upload progress tracking for the AWS .NET SDK S3 Transfer Utility by adding lifecycle events (initiated, completed, failed) and creating a unified upload response class. It introduces a comprehensive testing framework with JSON-based field mapping validation to ensure response completeness.
Key Changes:
- Added three new lifecycle events (
UploadInitiatedEvent,UploadCompletedEvent,UploadFailedEvent) toTransferUtilityUploadRequest - Created
TransferUtilityUploadResponseclass to unify response data from different upload operations - Implemented
ResponseMapperutility for mapping S3 response objects to the unified response format
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
TransferUtilityUploadRequest.cs |
Added lifecycle event handlers and event argument classes |
TransferUtilityUploadResponse.cs |
New unified response class with properties mapped from S3 operations |
SimpleUploadCommand.cs |
Added event firing logic and progress tracking |
SimpleUploadCommand.async.cs |
Added async event firing and response mapping |
ResponseMapper.cs |
New utility class for mapping S3 responses to unified format |
ResponseMapperTests.cs |
Comprehensive test suite with JSON-driven validation |
TransferUtilityTests.cs |
Integration tests for lifecycle events |
AssemblyInfo.cs |
Added InternalsVisibleTo for unit testing |
mapping.json |
JSON configuration defining field mappings between response types |
property-aliases.json |
Property name aliases for backward compatibility |
mapping-schema.json |
JSON schema validation for mapping configuration |
sdk/test/Services/S3/UnitTests/Custom/TestData/mapping-schema.json
Outdated
Show resolved
Hide resolved
sdk/test/Services/S3/UnitTests/Custom/TestData/mapping-schema.json
Outdated
Show resolved
Hide resolved
sdk/test/Services/S3/UnitTests/Custom/TestData/mapping-schema.json
Outdated
Show resolved
Hide resolved
sdk/test/Services/S3/UnitTests/Custom/TestData/mapping-schema.json
Outdated
Show resolved
Hide resolved
sdk/test/Services/S3/UnitTests/Custom/TestData/mapping-schema.json
Outdated
Show resolved
Hide resolved
sdk/test/Services/S3/UnitTests/Custom/TestData/mapping-schema.json
Outdated
Show resolved
Hide resolved
sdk/src/Services/S3/Custom/Transfer/Internal/SimpleUploadCommand.cs
Outdated
Show resolved
Hide resolved
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.
Pull Request Overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
| // Keep track of the total transferred bytes so that we can also return this value in case of failure | ||
| long transferredBytes = Interlocked.Add(ref _totalTransferredBytes, e.IncrementTransferred - e.CompensationForRetry); |
Copilot
AI
Oct 13, 2025
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.
The use of Interlocked.Add for thread-safe tracking of transferred bytes is correct, but consider potential race conditions where the final transferred bytes count might not accurately reflect the state at the exact moment of failure in multi-threaded scenarios.
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.
not sure if this really matters because this is supposed to be the current moment in time. also multi part upload has been doing this same logic
aws-sdk-net/sdk/src/Services/S3/Custom/Transfer/Internal/MultipartUploadCommand.cs
Line 372 in 9999d23
| long transferredBytes = Interlocked.Add(ref _totalTransferredBytes, e.IncrementTransferred - e.CompensationForRetry); |
25ac4ec to
80394b4
Compare
5f38ee4 to
b199735
Compare
04c81d1 to
4ff0353
Compare
0f0e9f4 to
352afda
Compare
1924fee to
db89c38
Compare
72d85c3 to
2dffdee
Compare
352afda to
a5de17c
Compare
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.
Pull Request Overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
| private void FireTransferInitiatedEvent() | ||
| { | ||
| var initiatedArgs = new UploadInitiatedEventArgs( | ||
| request: _fileTransporterRequest, | ||
| filePath: _fileTransporterRequest.FilePath, | ||
| totalBytes: _fileTransporterRequest.ContentLength | ||
| ); | ||
|
|
||
| _fileTransporterRequest.OnRaiseTransferInitiatedEvent(initiatedArgs); | ||
| } |
Copilot
AI
Oct 17, 2025
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.
totalBytes is sourced from _fileTransporterRequest.ContentLength, which is often 0/unspecified for file-path uploads and is set on the underlying PutObjectRequest, not on TransferUtilityUploadRequest. This causes incorrect TotalBytes (and will fail the new tests). Compute the size from the FilePath when present (FileInfo.Length), otherwise fall back to ContentLength (for stream uploads), e.g., introduce a helper GetTotalBytes() and use it here.
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.
this isnt a concern because in
| if (this.IsSetFilePath()) |
| FireTransferInitiatedEvent(); | ||
|
|
||
| var putRequest = ConstructRequest(); |
Copilot
AI
Oct 17, 2025
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.
[nitpick] Firing the initiated event before constructing the PutObjectRequest makes it harder to provide accurate metadata (like total bytes) if the file size or request normalization occurs during construction. Consider constructing or at least computing total size first, then firing the initiated event so the event carries accurate totals.
| FireTransferInitiatedEvent(); | |
| var putRequest = ConstructRequest(); | |
| var putRequest = ConstructRequest(); | |
| FireTransferInitiatedEvent(); |
| // Use invalid bucket name to force failure | ||
| var invalidBucketName = "invalid-bucket-name-" + Guid.NewGuid().ToString(); | ||
|
|
||
| try | ||
| { | ||
| UploadWithLifecycleEventsAndBucket(fileName, 5 * MEG_SIZE, invalidBucketName, null, null, eventValidator); | ||
| Assert.Fail("Expected an exception to be thrown for invalid bucket"); | ||
| } | ||
| catch (AmazonS3Exception) | ||
| { | ||
| // Expected exception - the failed event should have been fired | ||
| eventValidator.AssertEventFired(); | ||
| } |
Copilot
AI
Oct 17, 2025
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.
Using an invalid bucket name triggers client-side validation that can throw ArgumentException instead of AmazonS3Exception, making this test brittle. Use a valid but non-existent bucket name (e.g., conforming to S3 naming rules) and catch AmazonS3Exception, or broaden the catch to Exception to ensure the failed event assertion runs regardless of where the failure occurs.
generator/.DevConfigs/433a9a6d-b8ea-4676-b763-70711e8288e2.json
Outdated
Show resolved
Hide resolved
82ac159 to
810baa6
Compare
| this._fileTransporterRequest = fileTransporterRequest; | ||
|
|
||
| // Cache content length immediately while stream is accessible to avoid ObjectDisposedException in failure scenarios | ||
| this._cachedContentLength = this._fileTransporterRequest.ContentLength; |
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.
we actually do this in multi part upload command too
aws-sdk-net/sdk/src/Services/S3/Custom/Transfer/Internal/MultipartUploadCommand.cs
Line 83 in 9999d23
| this._contentLength = this._fileTransporterRequest.ContentLength; |
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.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
… upload update docs and function namse update comments add tests/update comments remove response from progressargs add missing properties update test Refactor test rearrange test update tests add commented out tests comments comments update comments fix spelling use interlocked read and fix build dev config update comment fix internals
3f70b05 to
9bd94c8
Compare
This change creates 3 event progress trackers for SimpleUploadCommand.
UploadInitiatedEvent,UploadedCompletedEvent, andUploadFailedEvent. This allows the user to subscribe to events like doing belowThese new events should only be fired 1 time during the entire lifecycle of the upload. The details of what each event should contain is in the SEP.
Description
Motivation and Context
This is being done as part of the SEP compliance.
Testing
Types of changes
Checklist
License