own request Bytes & to_json #145
Conversation
| @@ -13,7 +14,7 @@ where | |||
| ContentTypeSet: ToAssign, | |||
| { | |||
| attachment_client: &'a AttachmentClient, | |||
| body: Option<&'b [u8]>, | |||
| body: Option<Bytes>, | |||
There was a problem hiding this comment.
I'm not sure about this change. I don't really know how CreateSlugAttachmentBuilder is used. If It is reused, this should be a &Bytes then cloned.
There was a problem hiding this comment.
According to the docs (https://docs.rs/bytes/1.0.1/bytes/) bytes uses ref counting so cloning should be relatively cheap. I'd go with the owned struct.
| @@ -94,7 +95,7 @@ where | |||
| } | |||
|
|
|||
| impl<'a, 'b> CreateSlugAttachmentBuilder<'a, 'b, Yes, Yes> { | |||
| pub async fn execute(&self) -> Result<CreateSlugAttachmentResponse, CosmosError> { | |||
| pub async fn execute(self) -> Result<CreateSlugAttachmentResponse, CosmosError> { | |||
There was a problem hiding this comment.
Same here, if it is used many times, this should be changes back to &self.
There was a problem hiding this comment.
Generally speaking, the idea of taking &self in execute is to make it easier to implement retry (Azure services can be offline, throttled, etc...).
We do not have such code now but this is something we might want to implement (paired storage regions, HA endpoints in CosmosDB, etc...).
Also, for streaming requests, we could (theoretically) reuse the same struct at each request and just change the next marker.
You proposed change to use bytes partially solves the issue since it's refcounted anyway.
| ) -> Result<Response<Vec<u8>>, Box<dyn std::error::Error + Sync + Send>> { | ||
| let mut reqwest_request = | ||
| self.request(request.method().clone(), &request.uri().to_string()); | ||
| for header in request.headers() { | ||
| reqwest_request = reqwest_request.header(header.0, header.1); | ||
| } | ||
|
|
||
| let body = String::from_utf8(request.body().to_vec())?; |
There was a problem hiding this comment.
The problem here is that byte array reference gets cloned with to_vec & then converts it to a string, only to have the string converted to bytes later. It is more efficient to own the bytes and send them without the conversions.
There was a problem hiding this comment.
Yes you are right, the string conversion is not really necessary.
Also the idea to use ref counting here (by using bytes) is really good. 👍 👍
|
I really like this idea. |
| pub fn body(self, body: &'b [u8]) -> CreateSlugAttachmentBuilder<'a, 'b, Yes, ContentTypeSet> { | ||
| pub fn body( | ||
| self, | ||
| body: impl Into<Bytes>, |
There was a problem hiding this comment.
@rylev, like this with impl Into<Bytes>?
|
The storage rewrite in #119 was merged in, so I had to update storage as well. @MindFlavor, can you do another review please with the updates from c99bbc0? |
|
I like it, the examples showcase clearly how good this decision is! 👍 |
I'm looking to integrate the services with the http_client interface, but want to make a couple of adjustments before I do. Here is the first one. Instead of
serde_json::to_string,serde_json::to_vecis used. This avoids going from string to bytes later.