Skip to content

Add non-zero check for incremental upload buffer sizes#1543

Open
mansi153 wants to merge 2 commits intoawslabs:mainfrom
mansi153:zero-fix
Open

Add non-zero check for incremental upload buffer sizes#1543
mansi153 wants to merge 2 commits intoawslabs:mainfrom
mansi153:zero-fix

Conversation

@mansi153
Copy link
Contributor

@mansi153 mansi153 commented Jul 28, 2025

Add non-zero check for upload buffer-sizes.

This is done so that we avoid runtime exceptions for invalid sized buffers and division by 0 during capacity calculations in current or future incremental uploads. The buffer_size currently is derived from write-part-size. We have a check for read/write part-sizes being in the range [5MiB, 5GiB] in the CrtClientConfig. However, there are some tests and testing trait-implementations which don't do the same validation and hence need explicit checks in the upload constructor.

Does this change impact existing behavior?

No.

Does this change need a changelog entry? Does it require a version change?

No, minor code enhancement entry.


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and I agree to the terms of the Developer Certificate of Origin (DCO).

@mansi153 mansi153 had a problem deploying to PR integration tests July 28, 2025 09:44 — with GitHub Actions Failure
@mansi153 mansi153 had a problem deploying to PR integration tests July 28, 2025 09:44 — with GitHub Actions Failure
@mansi153 mansi153 had a problem deploying to PR integration tests July 28, 2025 09:44 — with GitHub Actions Failure
@mansi153 mansi153 had a problem deploying to PR integration tests July 28, 2025 09:44 — with GitHub Actions Failure
@mansi153 mansi153 had a problem deploying to PR integration tests July 28, 2025 09:44 — with GitHub Actions Failure
@mansi153 mansi153 had a problem deploying to PR integration tests July 28, 2025 09:44 — with GitHub Actions Failure
@mansi153 mansi153 had a problem deploying to PR integration tests July 28, 2025 09:44 — with GitHub Actions Failure
@mansi153 mansi153 had a problem deploying to PR integration tests July 28, 2025 09:44 — with GitHub Actions Failure
@mansi153 mansi153 had a problem deploying to PR integration tests July 28, 2025 09:44 — with GitHub Actions Failure
@mansi153 mansi153 had a problem deploying to PR integration tests July 28, 2025 09:44 — with GitHub Actions Failure
@mansi153 mansi153 had a problem deploying to PR integration tests July 28, 2025 09:44 — with GitHub Actions Failure
@mansi153 mansi153 had a problem deploying to PR integration tests July 28, 2025 09:44 — with GitHub Actions Failure
@mansi153 mansi153 had a problem deploying to PR integration tests July 28, 2025 09:44 — with GitHub Actions Failure
@mansi153 mansi153 had a problem deploying to PR integration tests July 28, 2025 09:44 — with GitHub Actions Failure
@mansi153 mansi153 had a problem deploying to PR integration tests July 28, 2025 09:44 — with GitHub Actions Failure
@mansi153 mansi153 had a problem deploying to PR integration tests July 28, 2025 09:44 — with GitHub Actions Failure
@mansi153 mansi153 had a problem deploying to PR integration tests July 28, 2025 09:44 — with GitHub Actions Failure
@mansi153 mansi153 had a problem deploying to PR integration tests July 28, 2025 09:44 — with GitHub Actions Failure
@mansi153 mansi153 had a problem deploying to PR integration tests July 28, 2025 09:44 — with GitHub Actions Failure
@mansi153 mansi153 had a problem deploying to PR integration tests July 28, 2025 09:44 — with GitHub Actions Failure
@mansi153 mansi153 had a problem deploying to PR integration tests July 28, 2025 09:44 — with GitHub Actions Failure
@muddyfish
Copy link
Contributor

Could you include the error message that's displayed when this fails in the description?

initial_offset: u64,
initial_etag: Option<ETag>,
) -> AppendUploadRequest<Client> {
assert!(self.buffer_size > 0, "write-part-size should be greater than 0 for incremental uploads");
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems the wrong place to check. Why not UploaderConfig::new (where buffer_size is passed)?

Also, the message here should be related to this type, rather than user-facing. --write-part-size is a CLI argument and should be validated elsewhere.

Copy link
Contributor

@c-hagem c-hagem Jul 28, 2025

Choose a reason for hiding this comment

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

Maybe we should consider making the size (NonZero)[https://doc.rust-lang.org/std/num/type.NonZeroU32.html]?

Signed-off-by: Mansi Pandey <[email protected]>
@mansi153 mansi153 had a problem deploying to PR integration tests July 28, 2025 14:22 — with GitHub Actions Failure
@mansi153 mansi153 had a problem deploying to PR integration tests July 28, 2025 14:22 — with GitHub Actions Failure
@mansi153 mansi153 had a problem deploying to PR integration tests July 28, 2025 14:22 — with GitHub Actions Failure
@mansi153 mansi153 had a problem deploying to PR integration tests July 28, 2025 14:22 — with GitHub Actions Failure
@mansi153 mansi153 had a problem deploying to PR integration tests July 28, 2025 14:22 — with GitHub Actions Failure
@mansi153 mansi153 had a problem deploying to PR integration tests July 28, 2025 14:22 — with GitHub Actions Failure
@mansi153 mansi153 had a problem deploying to PR integration tests July 28, 2025 14:22 — with GitHub Actions Failure
@mansi153 mansi153 had a problem deploying to PR integration tests July 28, 2025 14:22 — with GitHub Actions Failure
@mansi153 mansi153 had a problem deploying to PR integration tests July 28, 2025 14:22 — with GitHub Actions Failure
@mansi153 mansi153 had a problem deploying to PR integration tests July 28, 2025 14:22 — with GitHub Actions Failure
@mansi153 mansi153 had a problem deploying to PR integration tests July 28, 2025 14:22 — with GitHub Actions Failure
@mansi153 mansi153 had a problem deploying to PR integration tests July 28, 2025 14:22 — with GitHub Actions Failure
@mansi153 mansi153 had a problem deploying to PR integration tests July 28, 2025 14:22 — with GitHub Actions Failure
@mansi153 mansi153 had a problem deploying to PR integration tests July 28, 2025 14:22 — with GitHub Actions Failure
@mansi153 mansi153 had a problem deploying to PR integration tests July 28, 2025 14:22 — with GitHub Actions Failure
@mansi153 mansi153 had a problem deploying to PR integration tests July 28, 2025 14:22 — with GitHub Actions Failure
@mansi153 mansi153 had a problem deploying to PR integration tests July 28, 2025 14:22 — with GitHub Actions Failure
@mansi153 mansi153 had a problem deploying to PR integration tests July 28, 2025 14:22 — with GitHub Actions Failure
@mansi153 mansi153 had a problem deploying to PR integration tests July 28, 2025 14:22 — with GitHub Actions Failure
@mansi153 mansi153 had a problem deploying to PR integration tests July 28, 2025 14:22 — with GitHub Actions Failure
@mansi153 mansi153 had a problem deploying to PR integration tests July 28, 2025 14:22 — with GitHub Actions Failure
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.

4 participants