Skip to content

fix: S3 multipart upload redirect to correct region #4865

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

Merged
merged 2 commits into from
Aug 15, 2025

Conversation

kevinzwang
Copy link
Member

Changes Made

Do the same logic as the other S3 operations and retry multipart uploads with a new region if indicated by the error message to.

Related Issues

Checklist

  • Documented in API Docs (if applicable)
  • Documented in User Guide (if applicable)
  • If adding a new documentation page, doc is added to docs/mkdocs.yml navigation
  • Documentation builds and is formatted properly (tag @/ccmao1130 for docs review)

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

Greptile Summary

This PR implements region redirection logic for S3 multipart uploads to handle cases where the target S3 bucket is in a different region than the client's default region. The changes bring multipart upload operations in line with other S3 operations in the codebase that already handle region redirection.

The implementation adds retry functionality to three key multipart upload methods: create_multipart_upload, upload_part, and complete_multipart_upload. When these operations receive an HTTP 301 (moved permanently) response from S3, they now extract the correct region from the x-amz-bucket-region header and retry the operation with the new region parameter.

Key structural changes include:

  • A new MultipartUploadInfo struct that returns both the upload ID and the correct region from the create operation
  • Updated S3MultipartWriter to store and track the region throughout the multipart upload process
  • Addition of #[async_recursion] attributes to enable recursive retry attempts
  • Modified method signatures to accept region parameters

This change integrates seamlessly with the existing S3 implementation pattern, as the codebase already contains similar region redirection logic in get_impl, head_impl, list_impl, and put_impl methods. The multipart upload functionality was the missing piece that needed this cross-region capability.

Confidence score: 4/5

  • This is a well-structured fix that follows existing patterns in the codebase and addresses a clear functional gap
  • The implementation correctly mirrors the proven region redirection logic already used in other S3 operations
  • The src/daft-io/src/s3_like.rs file needs careful review to ensure all multipart upload edge cases are handled properly

1 file reviewed, no comments

Edit Code Review Bot Settings | Greptile

Copy link
Contributor

@desmondcheongzx desmondcheongzx left a comment

Choose a reason for hiding this comment

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

Noice

Copy link

codecov bot commented Jul 29, 2025

Codecov Report

❌ Patch coverage is 68.00000% with 24 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.34%. Comparing base (bf5d971) to head (5acb795).
⚠️ Report is 75 commits behind head on main.

Files with missing lines Patch % Lines
src/daft-io/src/s3_like.rs 68.00% 24 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4865      +/-   ##
==========================================
+ Coverage   79.19%   79.34%   +0.15%     
==========================================
  Files         893      893              
  Lines      124094   124626     +532     
==========================================
+ Hits        98277    98889     +612     
+ Misses      25817    25737      -80     
Files with missing lines Coverage Δ
src/daft-io/src/s3_like.rs 69.02% <68.00%> (-0.02%) ⬇️

... and 34 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@kevinzwang kevinzwang merged commit 072858e into main Aug 15, 2025
49 of 50 checks passed
@kevinzwang kevinzwang deleted the kevin/multipart-upload-region-change branch August 15, 2025 04:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants