Skip to content

Update TransferUtilityConfig and BaseDownloadRequest to add multipartdownload config options #4120

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: feature/transfermanager
Choose a base branch
from
+89 −1
Open

Update TransferUtilityConfig and BaseDownloadRequest to add multipartdownload config options #4120

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions generator/.DevConfigs/19ed68ce-9f46-4e1e-a0ff-45a2b3641946.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
{
"services": [
{
"serviceName": "S3",
"type": "patch",
"changeLogMessages": [
"Added MaxInMemoryParts property to TransferUtilityConfig for controlling memory usage during multipart downloads",
"Added PartSize property to BaseDownloadRequest for configuring multipart download part sizes",
"Added MultipartDownloadType enum and property to BaseDownloadRequest for selecting download strategy"
]
}
]
}
60 changes: 59 additions & 1 deletion sdk/src/Services/S3/Custom/Transfer/BaseDownloadRequest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,22 @@

namespace Amazon.S3.Transfer
{
/// <summary>
/// Specifies the strategy for multipart downloads
/// </summary>
public enum MultipartDownloadType
{
/// <summary>
/// Use part-based downloads with original upload part boundaries
/// </summary>
PART,

/// <summary>
/// Use range-based downloads with configurable part sizes
/// </summary>
RANGE
}

/// <summary>
/// The base class for requests that return Amazon S3 objects.
/// </summary>
Expand All @@ -50,6 +66,8 @@ public abstract class BaseDownloadRequest
private string ifMatch;
private string ifNoneMatch;
private ResponseHeaderOverrides responseHeaders;
private long? partSize;
private MultipartDownloadType multipartDownloadType = MultipartDownloadType.PART;

/// <summary>
/// Gets or sets the name of the bucket.
Expand Down Expand Up @@ -330,5 +348,45 @@ public ResponseHeaderOverrides ResponseHeaderOverrides
this.responseHeaders = value;
}
}

/// <summary>
/// Gets or sets the part size of the download in bytes.
/// The downloaded file will be divided into
/// parts the size specified and
/// downloaded from Amazon S3 individually.
/// This is used when MultipartDownloadType is set to RANGE.
/// </summary>
/// <value>
/// The part size of the download.
/// </value>
public long PartSize
{
get { return this.partSize.GetValueOrDefault(); }
set { this.partSize = value; }
Copy link

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

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

The PartSize property lacks input validation. Negative or zero values could lead to unexpected behavior when used for multipart downloads. Consider adding validation in the setter to ensure the value is positive, similar to the validation pattern used in TransferUtilityConfig.MaxInMemoryParts.

Suggested change
set { this.partSize = value; }
set
{
if (value <= 0)
{
throw new ArgumentOutOfRangeException(nameof(PartSize), "PartSize must be a positive value.");
}
this.partSize = value;
}

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

@GarrettBeatty GarrettBeatty Nov 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

we dont do this for multi part upload in this area

aws-sdk-net/sdk/src/Services/S3/Custom/Transfer/TransferUtilityUploadRequest.cs

Line 128 in 6203887

public long PartSize
not sure if this makes sense to do here, or have the validation later on

}

/// <summary>
/// Checks if PartSize property is set.
/// </summary>
/// <returns>true if PartSize property is set.</returns>
internal bool IsSetPartSize()
{
return this.partSize.HasValue;
}

/// <summary>
/// Gets or sets the type of multipart download to use.
/// PART: Uses part GET with original part sizes from upload (ignores PartSize)
/// RANGE: Uses ranged GET with PartSize to determine ranges
/// Default is PART
/// </summary>
/// <value>
/// The multipart download type.
/// </value>
public MultipartDownloadType MultipartDownloadType
{
get { return this.multipartDownloadType; }
set { this.multipartDownloadType = value; }
}
}
}
}
17 changes: 17 additions & 0 deletions sdk/src/Services/S3/Custom/Transfer/TransferUtilityConfig.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ public partial class TransferUtilityConfig
{
long _minSizeBeforePartUpload = 16 * (long)Math.Pow(2, 20);
int _concurrentServiceRequests;
int _maxInMemoryParts = 1024; // When combined with the default part size of 8MB, we get 8GB of memory being utilized as the default.

/// <summary>
/// Default constructor.
Expand Down Expand Up @@ -81,5 +82,21 @@ public int ConcurrentServiceRequests
this._concurrentServiceRequests = value;
}
}

/// <summary>
/// Gets or sets the maximum number of parts to buffer in memory during multipart downloads.
/// The default value is 1024.
/// </summary>
public int MaxInMemoryParts
{
get { return this._maxInMemoryParts; }
set
{
if (value < 1)
value = 1;
Comment on lines +90 to +96
Copy link

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

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

The setter silently modifies invalid input values instead of throwing an exception. This differs from standard .NET property validation patterns and can mask configuration errors. Consider throwing an ArgumentOutOfRangeException for values less than 1, similar to how the ConcurrentServiceRequests property's remarks indicate that invalid values are 'silently ignored' but the actual behavior is to clamp to a minimum. For consistency and clarity, either document this clamping behavior in the XML documentation or throw an exception for invalid values.

Suggested change
public int MaxInMemoryParts
{
get { return this._maxInMemoryParts; }
set
{
if (value < 1)
value = 1;
/// <remarks>
/// Throws <see cref="ArgumentOutOfRangeException"/> if the value is less than 1.
/// </remarks>
public int MaxInMemoryParts
{
get { return this._maxInMemoryParts; }
set
{
if (value < 1)
throw new ArgumentOutOfRangeException(nameof(value), "MaxInMemoryParts must be greater than 0.");

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

@GarrettBeatty GarrettBeatty Nov 11, 2025

Choose a reason for hiding this comment

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

wondering what the correct procedure is. i think argumentoutorrange makes sense but im not sure why we silently updated the value for concurrentservicerequests?


this._maxInMemoryParts = value;
}
}
}
}
Loading