Skip to content

Add an option to disable chunked encoding.#581

Merged
shorea merged 1 commit intoaws:masterfrom
harshavardhana:chunked-encoding
Dec 31, 2015
Merged

Add an option to disable chunked encoding.#581
shorea merged 1 commit intoaws:masterfrom
harshavardhana:chunked-encoding

Conversation

@harshavardhana
Copy link
Copy Markdown
Contributor

This is done to allow sha256 calculation of proper
payload in PutObjectRequest and UploadPartRequest.

Fixes #580

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

For your use case this class will never be used so this is fine but it may be confusing if another developer sets the configuration option and it isn't honored in the retry policy.

@harshavardhana
Copy link
Copy Markdown
Contributor Author

For your use case this class will never be used so this is fine but it may be confusing if another developer sets the configuration option and it isn't honored in the retry policy.

Are you saying i don't do this? - i need to create a separate overloaded class to avoid compiler errors. if you don't wish to set the value here.

the only reason i did that because i couldn't find a cleaner way to pass down s3ClientOptions. Should i pass this value in EndpointResolver? it doesn't look like the right place.

@harshavardhana harshavardhana force-pushed the chunked-encoding branch 2 times, most recently from a8d94cb to f75a905 Compare December 19, 2015 00:39
This is done to allow sha256 calculation of proper
payload in PutObjectRequest and UploadPartRequest.

Fixes aws#580
@shorea shorea self-assigned this Dec 28, 2015
@harshavardhana
Copy link
Copy Markdown
Contributor Author

Need to test this properly locally, please do not merge.

@shorea
Copy link
Copy Markdown
Contributor

shorea commented Dec 28, 2015

Hold off on testing. I had to rethink how this was implemented. I'll open a PR to show the changes and we can discuss if that meets your needs.

@harshavardhana
Copy link
Copy Markdown
Contributor Author

Hold off on testing. I had to rethink how this was implemented. I'll open a PR to show the changes and we can discuss if that meets your needs.

Oh okay cool.

@shorea
Copy link
Copy Markdown
Contributor

shorea commented Dec 29, 2015

Just created a pull request. During testing discovered this would not work for our SignerFactory as it expects a default constructor to be found and there's no clean way to get the S3ClientOptions to the Factory to instantiate the signer with the right configuration. The SignerFactory would be used to create the V4 signer if the signerRegionOverride was set. I'm guessing your customers are using the ENFORCE_SIG_V4 system property to enable SigV4 for S3 so you wouldn't have hit a code path that instantiates the SigV4 signer via SignerFactory. This change includes your new option in the request so that it's available no matter how the signer is created, via the factory or explicitly via a call to it's constructor. Let me know what you think and if this meets your needs. This PR isn't intended to be merged, if it's satisfactory I'll accept your PR and stage the changes internally.

@harshavardhana
Copy link
Copy Markdown
Contributor Author

Just created a pull request. During testing discovered this would not work for our SignerFactory as it expects a default constructor to be found and there's no clean way to get the S3ClientOptions to the Factory to instantiate the signer with the right configuration. The SignerFactory would be used to create the V4 signer if the signerRegionOverride was set. I'm guessing your customers are using the ENFORCE_SIG_V4 system property to enable SigV4 for S3 so you wouldn't have hit a code path that instantiates the SigV4 signer via SignerFactory. This change includes your new option in the request so that it's available no matter how the signer is created, via the factory or explicitly via a call to it's constructor. Let me know what you think and if this meets your needs. This PR isn't intended to be merged, if it's satisfactory I'll accept your PR and stage the changes internally.

This is perfect thank you +1 on your change.

@shorea
Copy link
Copy Markdown
Contributor

shorea commented Dec 29, 2015

Awesome! I'll run the integration tests over night and get this merged in tomorrow if all goes well.

@harshavardhana
Copy link
Copy Markdown
Contributor Author

Awesome! I'll run the integration tests over night and get this merged in tomorrow if all goes well.

Cool let me know if you need me to do something on my end.

@shorea
Copy link
Copy Markdown
Contributor

shorea commented Dec 29, 2015

Can you confirm in the comments of this PR that you are submitting the code under the Apache 2.0 license?

@harshavardhana
Copy link
Copy Markdown
Contributor Author

Can you confirm in the comments of this PR that you are submitting the code under the Apache 2.0 license?

Yes i confirm this patch is submitted under Apache 2.0 License.

@shorea
Copy link
Copy Markdown
Contributor

shorea commented Dec 30, 2015

BTW tests passed, just waiting for internal approval and I'll get this merged in.

@shorea
Copy link
Copy Markdown
Contributor

shorea commented Dec 31, 2015

Approved. Thanks for the contribution! This will be available in the first release of 2016 :)

shorea added a commit that referenced this pull request Dec 31, 2015
Add an option to disable chunked encoding.
@shorea shorea merged commit ead5459 into aws:master Dec 31, 2015
@harshavardhana harshavardhana deleted the chunked-encoding branch December 31, 2015 21:43
@harshavardhana
Copy link
Copy Markdown
Contributor Author

Approved. Thanks for the contribution! This will be available in the first release of 2016 :)

No thank you. :-)

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.

2 participants