Skip to content
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

Return both body and header for all AWS requests #116

Merged
merged 6 commits into from
Apr 14, 2020
Merged

Conversation

mattBrzezinski
Copy link
Member

AWSS3.jl is having issues where the multi-part upload is not working properly, it attempts to return request headers back as both the part number and etag are required to successfully complete a multi-part upload.

AWSCore.jl currently does not return headers unless the operation is a HEAD type request. This merge request returns back the response body and headers for all requests, which accommodates for these requirements.

This change will allow use to begin resolving AWSS3.jl #84.

@mattBrzezinski mattBrzezinski changed the title Return both body and header for all AWS requests WIP: Return both body and header for all AWS requests Apr 13, 2020
@mattBrzezinski mattBrzezinski changed the title WIP: Return both body and header for all AWS requests Return both body and header for all AWS requests Apr 13, 2020
@mattBrzezinski
Copy link
Member Author

bors try

bors bot added a commit that referenced this pull request Apr 13, 2020
@bors
Copy link
Contributor

bors bot commented Apr 13, 2020

try

Build succeeded:

@Keno
Copy link

Keno commented Apr 14, 2020

Good to go?

@mattBrzezinski
Copy link
Member Author

mattBrzezinski commented Apr 14, 2020

@omus any thoughts?

He's on vacation.

@iamed2
Copy link
Member

iamed2 commented Apr 14, 2020

We can avoid a breaking change here.

Instead, we could have an inner method that returns both, and have do_request keep the same interface. This will prevent the pain of updating requirements everywhere.

@mattBrzezinski
Copy link
Member Author

We can avoid a breaking change here.

Instead, we could have an inner method that returns both, and have do_request keep the same interface. This will prevent the pain of updating requirements everywhere.

Maybe it's too early in the morning, but I'm not following how this wouldn't be a breaking change. At some point we need the functionality to return back the headers and not just the response body.

@iamed2
Copy link
Member

iamed2 commented Apr 14, 2020

A slight tweak on my original idea:

Add a boolean headers argument to do_request, defaulting to false. If true, return the headers with the body, else keep the original interface.

Then this isn't a breaking change, and only the places that need to use the headers need to change.

@mattBrzezinski
Copy link
Member Author

bors try

bors bot added a commit that referenced this pull request Apr 14, 2020
@bors
Copy link
Contributor

bors bot commented Apr 14, 2020

try

Build succeeded:

@mattBrzezinski mattBrzezinski force-pushed the MB/s3-multi-part branch 4 times, most recently from 03fa742 to 8e4bd57 Compare April 14, 2020 19:27
test/aws4.jl Outdated Show resolved Hide resolved
Co-Authored-By: Eric Davies <iamed2@gmail.com>
@mattBrzezinski
Copy link
Member Author

bors r+

@bors
Copy link
Contributor

bors bot commented Apr 14, 2020

Build succeeded:

@bors bors bot merged commit 3283744 into master Apr 14, 2020
@bors bors bot deleted the MB/s3-multi-part branch April 14, 2020 20:14
end

# Return raw data by default...
return response.body
return (return_headers ? (response.body, nothing) : response.body)
Copy link

Choose a reason for hiding this comment

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

Why nothing here? This is the codepath I'm seeing AWSS3 take with your changes, resulting in:

MethodError: no method matching getindex(::Nothing, ::String)
Stacktrace:
 [1] s3_upload_part(::Dict{Symbol,Any}, ::XMLDict.XMLDictElement, ::Int64, ::Array{UInt8,1}) at /home/keno/.julia/dev/AWSS3/src/AWSS3.jl:698

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in #117 , I think the comment above it threw me off.

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