Skip to content

Conversation

@GarrettBeatty
Copy link
Contributor

@GarrettBeatty GarrettBeatty commented Nov 6, 2025

Stacked PRs:


Description

Create TransferUtililityDownloadResponse class which is populated with the number of successful downloads.

Motivation and Context

  1. Eventually we will be creating new DownloadWithResponseAPI that will return this object.

Testing

  1. dry run in progress 3588c4be-0ad1-4585-bc7e-34ec32c64841

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My code follows the code style of this project
  • My change requires a change to the documentation
  • I have updated the documentation accordingly
  • I have read the README document
  • I have added tests to cover my changes
  • All new and existing tests passed

License

  • I confirm that this pull request can be released under the Apache 2 license

GarrettBeatty added a commit that referenced this pull request Nov 6, 2025
…downloaded

stack-info: PR: #4109, branch: GarrettBeatty/stacked/14
@GarrettBeatty GarrettBeatty force-pushed the GarrettBeatty/stacked/14 branch from 8b602c7 to fe575d5 Compare November 6, 2025 21:36
@GarrettBeatty GarrettBeatty changed the base branch from GarrettBeatty/stacked/10 to feature/transfermanager November 6, 2025 21:46
GarrettBeatty added a commit that referenced this pull request Nov 6, 2025
…downloaded

stack-info: PR: #4109, branch: GarrettBeatty/stacked/14
@GarrettBeatty GarrettBeatty force-pushed the GarrettBeatty/stacked/14 branch from fe575d5 to ca38dab Compare November 6, 2025 21:46
@GarrettBeatty GarrettBeatty changed the base branch from feature/transfermanager to GarrettBeatty/stacked/10 November 6, 2025 21:46
var command = new DownloadCommand(this._s3Client, downloadRequest);

var task = ExecuteCommandAsync(command, internalCts, asyncThrottler);
var task = ExecuteCommandAsync(command, internalCts, asyncThrottler)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what im trying to do here is simply "increment value" when task completes

@GarrettBeatty GarrettBeatty requested a review from Copilot November 6, 2025 21:49
Copy link
Contributor

Copilot AI left a 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 functionality to track and report the number of successfully downloaded objects in the TransferUtilityDownloadDirectoryResponse. The implementation populates the ObjectsDownloaded property by counting completed download tasks using thread-safe increment operations.

Key changes:

  • Added ObjectsDownloaded property to TransferUtilityDownloadDirectoryResponse
  • Modified DownloadDirectoryCommand.ExecuteAsync to track successful downloads using a thread-safe counter
  • Added dev config for S3 service with patch version bump

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File Description
sdk/src/Services/S3/Custom/Transfer/TransferUtilityDownloadDirectoryResponse.cs Added ObjectsDownloaded property to track the number of successfully downloaded objects
sdk/src/Services/S3/Custom/Transfer/Internal/_bcl+netstandard/DownloadDirectoryCommand.cs Implemented counter for successful downloads using task continuations and thread-safe increment
generator/.DevConfigs/c49077d9-90b3-437f-b316-6d8d8833ae73.json Added dev config specifying this as a patch version change for S3 service

Comment on lines +25 to +28
/// <summary>
/// The number of objects that have been downloaded
/// </summary>
public long ObjectsDownloaded { get; set; }
Copy link

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ObjectsDownloaded property is not accessible to API consumers because the public DownloadDirectoryAsync and DownloadDirectory methods do not return the response object. The internal command returns TransferUtilityDownloadDirectoryResponse, but the public API methods have return type Task (async) or void (sync), discarding the response.

To make this property useful, the public API signatures should be updated to return Task<TransferUtilityDownloadDirectoryResponse> and TransferUtilityDownloadDirectoryResponse respectively. If this is planned for a future change, consider documenting this as work-in-progress. If the API signature cannot be changed due to backward compatibility, consider an alternative mechanism to expose this information.

Copilot generated this review using guidance from repository custom instructions.
"services": [
{
"serviceName": "S3",
"type": "patch",
Copy link

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The dev config specifies this as a "patch" change, but this introduces a new public property (ObjectsDownloaded) on TransferUtilityDownloadDirectoryResponse, which is a new feature. According to CONTRIBUTING.md guidelines, new features should be marked as "minor" rather than "patch".

Additionally, if the public API signatures are updated to return the response object (which would be necessary to make this property accessible), that would also constitute a minor version change as it's a backward-compatible API enhancement.

Copilot generated this review using guidance from repository custom instructions.

SemaphoreSlim asyncThrottler = null;
CancellationTokenSource internalCts = null;
int successfulDownloads = 0;
Copy link

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The type of successfulDownloads should be long to match the ObjectsDownloaded property type. While implicit conversion from int to long works, using consistent types improves code clarity and prevents potential overflow issues if the number of downloads exceeds int.MaxValue.

Suggested change
int successfulDownloads = 0;
long successfulDownloads = 0;

Copilot uses AI. Check for mistakes.
@GarrettBeatty GarrettBeatty changed the base branch from GarrettBeatty/stacked/10 to feature/transfermanager November 6, 2025 22:05
GarrettBeatty added a commit that referenced this pull request Nov 6, 2025
…downloaded

stack-info: PR: #4109, branch: GarrettBeatty/stacked/14
@GarrettBeatty GarrettBeatty force-pushed the GarrettBeatty/stacked/14 branch from ca38dab to a7612ae Compare November 6, 2025 22:05
@GarrettBeatty GarrettBeatty changed the base branch from feature/transfermanager to GarrettBeatty/stacked/10 November 6, 2025 22:06
@GarrettBeatty GarrettBeatty changed the base branch from GarrettBeatty/stacked/10 to feature/transfermanager November 7, 2025 14:40
GarrettBeatty added a commit that referenced this pull request Nov 7, 2025
…downloaded

stack-info: PR: #4109, branch: GarrettBeatty/stacked/14
@GarrettBeatty GarrettBeatty force-pushed the GarrettBeatty/stacked/14 branch from a7612ae to 3399d67 Compare November 7, 2025 14:40
@GarrettBeatty GarrettBeatty changed the base branch from feature/transfermanager to GarrettBeatty/stacked/10 November 7, 2025 14:40
Base automatically changed from GarrettBeatty/stacked/10 to feature/transfermanager November 7, 2025 14:41
…downloaded

stack-info: PR: #4109, branch: GarrettBeatty/stacked/14
@GarrettBeatty GarrettBeatty force-pushed the GarrettBeatty/stacked/14 branch from 3399d67 to 3c74513 Compare November 7, 2025 16:49
@GarrettBeatty GarrettBeatty marked this pull request as ready for review November 7, 2025 16:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant