Encode request payload optionally with Gzip #1615

plazma-prizma · 2019-11-22T14:14:58Z

Allow content encoding for compressing request payloads with Gzip

Continuation of #1469 PR discussion.

I'll generate services in separate commit.

Added an integration test for CloudWatch logs to support the use case also.

Rebase rusoto master

merge version 42

luben · 2019-11-22T14:55:18Z

rusoto/core/src/client.rs

@@ -35,6 +36,7 @@ impl Client {
        let inner = Arc::new(ClientInner {
            credentials_provider: Some(Arc::new(credentials_provider)),
            dispatcher: Arc::new(dispatcher),
+            content_encoding: Default::default(),


what if we move the content_encoding to the Client and pass it to Inner methods only when needed.

I checked it out and I need to call encode() in SignAndDispatch trait and ClientInner implements it, not Client.

So than may be add new constructor, or may be a builder. This way we don't need to mutate it.

I have implemented in that way, it is better because we don't need to regenerate other services. Take a look at the new version.

plazma-prizma · 2019-11-22T16:34:37Z

rusoto/core/src/lib.rs

@@ -35,6 +35,7 @@ pub extern crate rusoto_credential as credential;
 extern crate serde;
 #[macro_use]
 extern crate serde_derive;
+pub extern crate flate2;


What do you think about it? People will access it as rusoto_core::flate2

I don't think we need to make it pub. If people need it they will defined their own dependencies.

luben · 2019-11-23T22:51:51Z

rusoto/core/src/client.rs

+
+    /// Set content_encoding type for http requests which will compress request payload accordingly
+    pub fn set_http_content_encoding(&mut self, content_encoding: ContentEncoding) {
+        let inner = Arc::get_mut(&mut self.inner)


this is the main problem I see - sometimes it will succeed, sometimes it will panic, depending on the number of references.

plazma-prizma · 2019-11-24T22:15:33Z

integration_tests/tests/ds.rs

@@ -28,28 +28,28 @@ fn should_describe_directories() {
 fn should_conditional_forwarders() {
    let client = DirectoryServiceClient::new(Region::UsWest2);
    let mut request = DescribeConditionalForwardersRequest::default();
-	request.directory_id = "d-11111aaaaa".to_string();
+    request.directory_id = "d-11111aaaaa".to_string();


these are from cargo fmt

plazma-prizma · 2019-11-25T13:28:57Z

Hey @matthewkmayer @iliana could you guys check it out?

plazma-prizma · 2019-11-28T16:33:29Z

ping

matthewkmayer · 2019-11-28T18:17:18Z

rusoto/core/Cargo.toml

@@ -42,6 +42,7 @@ time = "0.1.35"
 tokio = "0.1.7"
 tokio-timer = "0.2.6"
 xml-rs = "0.8"
+flate2 = "1.0"


Can we change this dependency so it's only brought in when the encoding flag is active?

flate2 = { version = "1.0", optional = true }

matthewkmayer · 2019-11-28T18:18:13Z

rusoto/core/Cargo.toml

@@ -63,6 +64,7 @@ serde_test = "1.0.1"

 [features]
 default = ["native-tls"]
+encoding = []


To achieve the behavior in the comment above, this can be changed to encoding = ["flate2"] to pull in the crate as needed. 👍

matthewkmayer · 2019-11-28T18:20:46Z

integration_tests/Cargo.toml

@@ -24,6 +24,9 @@ debug = false
 path = "../rusoto/core"
 default-features = false

+[dependencies.rusoto_credential]
+path = "../rusoto/credential"


I think we can skip this change, since rusoto_core reexports rusoto_credential. use rusoto_core::credential::DefaultCredentialsProvider; should do the trick. 👍

matthewkmayer · 2019-11-28T18:30:23Z

rusoto/core/src/encoding.rs

+                        let mut encoder = GzEncoder::new(Vec::<u8>::new(), Compression::new(*level));
+                        encoder
+                            .write_all(payload)
+                            .expect("Request payload was not written to encoder.");


While it's been my experience flate2 doesn't return errors in real use, what do you think about returning the errors instead of using unwrap or expect? That way we can pass the errors back up to the client and the caller can handle it from there.

I'm okay with that change being done in a followup PR, as this one is close to being done. 😄

Thanks for the review, I'll address it on the next PR :)

matthewkmayer · 2019-11-28T18:31:30Z

Thanks for sticking with this! It's only got a couple changes to make, per my comments, and it should be good to go. 🎉

* master: (162 commits) Fix doc test for Secret's Debug impl. Add rusoto#1615 to changelog Fixes - Changes according to feedback from Iliana - Fix hyperlink in documentation - Changes based on feedback from first review. Updated changelog Add custom dependency for STS (didn't realise that I cannot just change the dependencies in Cargo.toml). Added new DefaultCredentialsProvider implementation in rusoto_sts which supports more credentials sources. Implemented WebIdentityProvider to support IAM Roles for Kubernetes Service Accounts (https://docs.aws.amazon.com/eks/latest/userguide/iam-roles-for-service-accounts.html). update changelog for fix sns api fix ignore key-value order Limit publishing rate by reducing parallel compilations. add custom test for sns api This clippy lint should be ignored in crategen. Set rusoto_mock to same version as rusoto_core in crategen. Publish crates in alphabetical order. Don't run doctests on nightly. Remove flate2 dependency from core Implement encoding client with a separate constructor ...

plazma-prizma and others added 5 commits November 18, 2019 15:09

Merge pull request #1 from rusoto/master

27a8154

Rebase rusoto master

Merge pull request #2 from rusoto/master

1206ce4

merge version 42

Allow encoding request payload for compression

35e4045

Fix small cosmetic issues

a495b71

Other fixes

7a0871c

plazma-prizma changed the title ~~Encode request payload optionally with Gzip~~ [WIP]Encode request payload optionally with Gzip Nov 22, 2019

luben reviewed Nov 22, 2019

View reviewed changes

plazma-prizma commented Nov 22, 2019

View reviewed changes

luben reviewed Nov 23, 2019

View reviewed changes

plazma-prizma added 3 commits November 24, 2019 11:49

Run cargo fmt on integration tests

25b7891

Run cargo fmt on integration tests

61f5472

Implement encoding client with a separate constructor

7795286

plazma-prizma commented Nov 24, 2019

View reviewed changes

plazma-prizma changed the title ~~[WIP]Encode request payload optionally with Gzip~~ Encode request payload optionally with Gzip Nov 25, 2019

Remove flate2 dependency from core

8a18d7e

matthewkmayer reviewed Nov 28, 2019

View reviewed changes

matthewkmayer self-assigned this Nov 28, 2019

matthewkmayer reviewed Nov 28, 2019

View reviewed changes

plazma-prizma added 2 commits December 4, 2019 14:05

Fixes

6e29eff

Add rusoto#1615 to changelog

9e07435

iliana merged commit f45f267 into rusoto:master Dec 5, 2019

iliana mentioned this pull request Dec 5, 2019

[WIP] Compressor approach #1470

Closed

matthewkmayer mentioned this pull request Dec 8, 2019

Send compressed payload by gzip #1461

Closed

fanatid mentioned this pull request May 23, 2020

Fix Content-Encoding applying in Client #1757

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Encode request payload optionally with Gzip #1615

Encode request payload optionally with Gzip #1615

plazma-prizma commented Nov 22, 2019

luben Nov 22, 2019

plazma-prizma Nov 22, 2019

luben Nov 23, 2019

plazma-prizma Nov 24, 2019

plazma-prizma Nov 22, 2019

luben Nov 22, 2019 •

edited

Loading

luben Nov 23, 2019

plazma-prizma Nov 24, 2019

plazma-prizma commented Nov 25, 2019

plazma-prizma commented Nov 28, 2019

matthewkmayer Nov 28, 2019

matthewkmayer Nov 28, 2019

matthewkmayer Nov 28, 2019 •

edited

Loading

matthewkmayer Nov 28, 2019

plazma-prizma Dec 5, 2019

matthewkmayer commented Nov 28, 2019

Encode request payload optionally with Gzip #1615

Encode request payload optionally with Gzip #1615

Conversation

plazma-prizma commented Nov 22, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luben Nov 22, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

plazma-prizma commented Nov 25, 2019

plazma-prizma commented Nov 28, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthewkmayer Nov 28, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthewkmayer commented Nov 28, 2019

luben Nov 22, 2019 •

edited

Loading

matthewkmayer Nov 28, 2019 •

edited

Loading