WIP: feat(attachment): Inline small attachments instead of uploading to objectstore#6165
WIP: feat(attachment): Inline small attachments instead of uploading to objectstore#6165tobias-wilfert wants to merge 1 commit into
Conversation
| #[serde( | ||
| rename = "relay.attachment-inline.limit", | ||
| deserialize_with = "default_on_error", | ||
| skip_serializing_if = "is_default" | ||
| )] | ||
| pub attachment_inline_limit: u32, | ||
|
|
There was a problem hiding this comment.
Idea here is that we can roll this change out as is (NOOP) and once we set this value it would take effect. Would also allow us to experiment with different values.
There was a problem hiding this comment.
Talked with Joris and David offline and making this a usize should be fine.
| } | ||
| } | ||
| } else { | ||
| let minidump_data = request.extract().await?; |
There was a problem hiding this comment.
Follow up: I think we need to add the same size regulating logic that we already have in read_field_into_item here.
There was a problem hiding this comment.
Can you create an issue for this so we don't forget?
| match upload_context.upload_decision(item.attachment_type()) { | ||
| UploadDecision::Inline => read_inline(field, item).await, | ||
| UploadDecision::Upload => { | ||
| let content_type = field.content_type().map(|ct| ct.essence_str().to_owned()); |
There was a problem hiding this comment.
I think we want to use essence_str in general (we currently do it differently at a couple of spots in the new logi so I think we want to fix that in a follow up).
| /// Rationale being that if the attachment is smaller than the attachment reference, obtained by | ||
| /// uploading, there is no point in uploading. |
There was a problem hiding this comment.
| /// Rationale being that if the attachment is smaller than the attachment reference, obtained by | |
| /// uploading, there is no point in uploading. | |
| /// If the attachment is smaller than the attachment reference obtained by | |
| /// uploading, there is no point in uploading. |
| } | ||
| } | ||
| } else { | ||
| let minidump_data = request.extract().await?; |
There was a problem hiding this comment.
Can you create an issue for this so we don't forget?
| /// The result of [`split_by_size`]. | ||
| pub enum SizeSplit<S> { | ||
| /// The stream is less than limit and hence buffered into memory. | ||
| Small(Bytes), | ||
| /// The stream exceeds limit and hence is not buffered. | ||
| Large(S), | ||
| } | ||
|
|
||
| /// Buffers a stream if its size is less than `limit` bytes, else return the stream. | ||
| pub async fn split_by_size<S, E>( | ||
| stream: S, | ||
| limit: usize, | ||
| ) -> Result<SizeSplit<impl Stream<Item = Result<Bytes, E>> + Send>, E> | ||
| where | ||
| S: Stream<Item = Result<Bytes, E>> + Send, | ||
| E: Send, | ||
| { | ||
| let (head, stream) = peek_n(stream, limit).await?; | ||
| if head.len() < limit { | ||
| Ok(SizeSplit::Small(head)) | ||
| } else { | ||
| Ok(SizeSplit::Large(stream)) | ||
| } | ||
| } |
There was a problem hiding this comment.
I believe you can simplify the API by changing peek_n to return (Bytes, Option<Stream>), i.e. return None for the stream if there's nothing after head. reject_if_compressed can re-use the bytes obtained by the first call to peek_n.
2 participants
Fix: RELAY-255