Improve Copy Operation by taking the Source URI #482
Conversation
waahm7
changed the title
waahm7
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #482 +/- ##
==========================================
- Coverage 89.64% 89.61% -0.04%
==========================================
Files 20 20
Lines 6144 6287 +143
==========================================
+ Hits 5508 5634 +126
- Misses 636 653 +17
|
| AWS_LS_S3_META_REQUEST, | ||
| "Unable to parse the copy_source_uri provided in the request: " PRInSTR "", | ||
| AWS_BYTE_CURSOR_PRI(options->copy_source_uri)); | ||
| goto on_error; |
There was a problem hiding this comment.
the copy_object->synced_data.part_list was initialized above, it needs to be cleaned up here.
Or move the initialization of the list after anything can fail
We could add a simple test to pass in an invalid URI to add a bit more test coverage.
There was a problem hiding this comment.
Thanks, good catch. Fixed with a test.
source/s3_request_messages.c
Outdated
| struct aws_http_message *message = NULL; | ||
|
|
||
| message = aws_http_message_new_request(allocator); |
There was a problem hiding this comment.
Lot of the error handling down there assumes there was nothing to clean up and just returns null.
Need to update them since we now allocate the message at the beginning of the function.
|
|
||
| message = aws_http_message_new_request(allocator); | ||
| if (message == NULL) { | ||
| goto error_cleanup; |
There was a problem hiding this comment.
head_object_host_header have not been defined yet, but we will try to clean it up from this goto.
| struct aws_byte_buf head_object_host_header; | ||
| AWS_ZERO_STRUCT(head_object_host_header); | ||
|
|
||
| if (message == NULL) { |
There was a problem hiding this comment.
do we need to worry about the signing region here today as well? or we are keeping the source must be in the same region as dest behavior?
There was a problem hiding this comment.
Good catch. For now, I have updated the docs to mention that only limitations 1 and 2 will be bypassed using URI.
Issue #, if available:
awslabs/s3-connector-for-pytorch#295
Description of changes:
The current copy implementation has a lot of limitations because we try to parse the source host and path from the
x-amz-copy-sourceheader and the destination URI. This falls apart in many cases, such as when the bucket has a.in its name or during cross-region copies where we can't generate the source URL correctly due to missing information. Allowing the user to pass the source URL can bypass these limitations.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.