Conversation
tatiesmars
force-pushed
the
feature/add-multipart
branch
from
11ba7da to
e37381b
Compare
tatiesmars
force-pushed
the
feature/add-multipart
branch
from
e37381b to
1d989dd
Compare
shuning-auki
left a comment
shuning-auki
left a comment
There was a problem hiding this comment.
The problem is not new in this PR, but I just noticed, existing upload endpoints rolls back all changes when there is an error, but combining upload and create together makes it confusing which part has actually failed and how to retry. Same problem with new multipart endpoints, now each upload is one transaction, it makes it even harder for clients to know which one should be retried if something failed
Co-authored-by: shuning <shuning@aukilabs.com>
Co-authored-by: shuning <shuning@aukilabs.com>
|
I think the multipart in it self works fine from my tests, when things fail it is because of infra configuration in the environment. |
Improvements have been made to allow for larger ephemeral volumes when domain servers are running without local storage. These improvements has been rolled out in our dev environment so you can test again :) |
There was a problem hiding this comment.
Pull request overview
This PR adds multipart upload support to handle large data uploads more efficiently. The implementation includes server capability detection via /api/v1/info endpoint, a caching mechanism for server info, and fallback logic to maintain backward compatibility.
Key Changes:
- Added multipart upload protocol with initiate/upload/complete/abort endpoints
- Implemented server capability detection with 60-second caching
- Enhanced upload functions to automatically use multipart for oversized payloads
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| core/domain-http/src/domain_data.rs | Implements multipart upload protocol, server info caching, and integrates multipart logic into existing upload streams |
| core/domain-http/Cargo.toml | Version bump to 1.5.0 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| use bytes::Bytes; | ||
| use futures::{SinkExt, Stream, channel::mpsc, stream::StreamExt}; | ||
| use reqwest::{Body, Client, Response}; | ||
| use futures::lock::Mutex; |
There was a problem hiding this comment.
Using futures::lock::Mutex can cause performance issues in async contexts. Consider using tokio::sync::Mutex instead, which is optimized for async/await and provides better performance in Tokio runtime environments.
|
|
||
| struct Batch { | ||
| tx: mpsc::Sender<Vec<u8>>, | ||
| done: oneshot::Receiver<Result<Vec<DomainDataMetadata>, DomainError>>, |
There was a problem hiding this comment.
The oneshot channel is used but not imported. Add use futures::channel::oneshot; or use tokio::sync::oneshot; to the imports.
| } | ||
|
|
||
| let (name, data_type, existing_id) = match action { | ||
| DomainAction::Create { name, data_type } => (name, data_type, None), |
There was a problem hiding this comment.
Using empty strings for name and data_type in the Update action is unclear. Consider using more explicit placeholder values or adding a comment explaining why these fields are empty for updates.
| DomainAction::Create { name, data_type } => (name, data_type, None), | |
| DomainAction::Create { name, data_type } => (name, data_type, None), | |
| // For updates, `name` and `data_type` are determined by the existing record on the server, | |
| // so we intentionally send empty strings and only populate `existing_id`. |
|
|
||
| // If we can't determine a meaningful request size limit, keep the existing streaming behavior. | ||
| if request_max_bytes <= 0 || !multipart_enabled { |
There was a problem hiding this comment.
The condition request_max_bytes <= 0 treats 0 and negative values the same way. Since request_max_bytes is i64, consider explicitly checking for values less than or equal to zero separately, or document why negative values are treated the same as zero.
| // If we can't determine a meaningful request size limit, keep the existing streaming behavior. | |
| if request_max_bytes <= 0 || !multipart_enabled { | |
| let has_meaningful_request_limit = request_max_bytes > 0; | |
| // If we can't determine a meaningful request size limit, keep the existing streaming behavior. | |
| if !has_meaningful_request_limit || !multipart_enabled { |
| } | ||
| Err(e) => { | ||
| if is_unsupported_endpoint_error(&e) { | ||
| // Endpoint not supported: fall back to single upload (will likely 413). |
There was a problem hiding this comment.
The comment mentions '(will likely 413)' but doesn't explain what should happen if that occurs. Consider clarifying the expected behavior or error handling when the fallback also fails with a 413 status.
| // Endpoint not supported: fall back to single upload (will likely 413). | |
| // Endpoint not supported: fall back to single upload (which may fail with 413; in that case the error is propagated to the caller). |
No description provided.