-
Notifications
You must be signed in to change notification settings - Fork 867
Add DownloadInitiated, Failed and Completed events #4079
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: GarrettBeatty/stacked/4
Are you sure you want to change the base?
Conversation
stack-info: PR: #4079, branch: GarrettBeatty/stacked/10
1485aec to
ee59a30
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
This PR adds lifecycle events (DownloadInitiatedEvent, DownloadCompletedEvent, and DownloadFailedEvent) to the S3 TransferUtility download functionality, providing developers with better visibility into download operations.
Key changes:
- New event handlers and event argument classes for download lifecycle events
- Integration of events into the DownloadCommand execution flow
- Comprehensive integration tests covering individual and combined lifecycle events
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
sdk/src/Services/S3/Custom/Transfer/TransferUtilityDownloadRequest.cs |
Adds three new public events (DownloadInitiatedEvent, DownloadCompletedEvent, DownloadFailedEvent) with corresponding event argument classes and internal event-raising methods |
sdk/src/Services/S3/Custom/Transfer/Internal/_async/DownloadCommand.async.cs |
Implements event firing at appropriate points in the download lifecycle, tracks total bytes from response headers, and maps the final response |
sdk/src/Services/S3/Custom/Transfer/Internal/DownloadCommand.cs |
Adds event firing helper methods and tracks total transferred bytes using Interlocked operations for thread safety |
sdk/src/Services/S3/Custom/Model/GetObjectResponse.cs |
Adds Request property to WriteObjectProgressArgs to provide access to the original download request |
sdk/test/Services/S3/IntegrationTests/TransferUtilityTests.cs |
Adds five new integration tests covering individual events and complete lifecycle scenarios |
generator/.DevConfigs/9d07dc1e-d82d-4f94-8700-c7b57f872123.json |
Adds dev config for version bump |
generator/.DevConfigs/9d07dc1e-d82d-4f94-8700-c7b57f872123.json
Outdated
Show resolved
Hide resolved
ee59a30 to
e553dbb
Compare
stack-info: PR: #4079, branch: GarrettBeatty/stacked/10
stack-info: PR: #4079, branch: GarrettBeatty/stacked/10
e553dbb to
8782802
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 6 out of 6 changed files in this pull request and generated 2 comments.
sdk/src/Services/S3/Custom/Transfer/Internal/_async/DownloadCommand.async.cs
Outdated
Show resolved
Hide resolved
stack-info: PR: #4079, branch: GarrettBeatty/stacked/10
8782802 to
b0fa14b
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 6 out of 6 changed files in this pull request and generated 3 comments.
sdk/src/Services/S3/Custom/Transfer/Internal/_async/DownloadCommand.async.cs
Outdated
Show resolved
Hide resolved
sdk/src/Services/S3/Custom/Transfer/Internal/_async/DownloadCommand.async.cs
Outdated
Show resolved
Hide resolved
stack-info: PR: #4079, branch: GarrettBeatty/stacked/10
b0fa14b to
a92a064
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 6 out of 6 changed files in this pull request and generated 4 comments.
sdk/src/Services/S3/Custom/Transfer/Internal/_async/DownloadCommand.async.cs
Show resolved
Hide resolved
stack-info: PR: #4079, branch: GarrettBeatty/stacked/10
a92a064 to
3e702bb
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 6 out of 6 changed files in this pull request and generated 2 comments.
| /// <param name="transferredBytes">The total number of bytes transferred</param> | ||
| /// <param name="totalBytes">The total number of bytes for the complete file</param> | ||
| internal DownloadCompletedEventArgs(TransferUtilityDownloadRequest request, TransferUtilityDownloadResponse response, string filePath, long transferredBytes, long totalBytes) | ||
| { |
Copilot
AI
Oct 24, 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 response parameter could potentially be null based on the calling code in DownloadCommand.async.cs line 148. Consider adding null validation in the constructor or documenting that a null response should never be passed, to prevent potential issues if the calling code changes.
| { | |
| { | |
| if (response == null) | |
| throw new ArgumentNullException(nameof(response), "Response cannot be null."); |
| UtilityMethods.GenerateFile(originalFilePath, size); | ||
|
|
||
| Client.PutObject(new PutObjectRequest |
Copilot
AI
Oct 24, 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 DownloadWithLifecycleEvents and DownloadWithLifecycleEventsAndKey helper methods contain duplicated code for setting up event handlers (lines 2282-2295 and 2315-2328). Consider extracting the event handler setup into a shared helper method to reduce duplication and improve maintainability.
| return new TransferUtilityDownloadResponse(); | ||
| FireTransferCompletedEvent(lastSuccessfulMappedResponse, this._request.FilePath, Interlocked.Read(ref _totalTransferredBytes), totalBytesFromResponse ?? -1); | ||
|
|
||
| return lastSuccessfulMappedResponse; |
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 is saying that this could potentially be null but based on my reading of the code, if there is any failure we will throw exception early on, so it can never be null. thoughts?
stack-info: PR: #4079, branch: GarrettBeatty/stacked/10
3e702bb to
0209113
Compare
Stacked PRs:
Description
This change adds progress listeners for "initiated", "complete", and "failed" lifecycle events for the DownloadCommand Consumers can subscribe to these callbacks to receive notifications when a simple upload starts, finishes successfully, or fails. Its similar to #4059 but for downloads
Motivation and Context
Testing
f0cb4050-fb2c-4fb0-b489-280eb3313ca9- in progressTypes of changes
Checklist
License