-
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
base: gcbeatty/tuuploadresponse
Are you sure you want to change the base?
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
TransferUtilityUploadResponse
class to unify response data from different upload operations - Implemented
ResponseMapper
utility 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/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.
Copilot uses AI. Check for mistakes.
// Navigate up from bin/Debug/net472 to the test project root | ||
while (testProjectDirectory != null && !File.Exists(Path.Combine(testProjectDirectory, "Custom", "TestData", "mapping.json"))) |
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 hardcoded navigation logic to find test data files is fragile and could break with different build configurations or directory structures. Consider using a more robust approach like [DeploymentItem]
attribute or embedded resources.
Copilot uses AI. Check for mistakes.
25ac4ec
to
80394b4
Compare
5f38ee4
to
b199735
Compare
1c63392
to
04c81d1
Compare
… GetObjectResponse
04c81d1
to
4ff0353
Compare
b511b03
to
0f0e9f4
Compare
… 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
0f0e9f4
to
352afda
Compare
1924fee
to
db89c38
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