feat: parallel chunkedUpload in web and node #1146
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Pull Request Overview
This PR upgrades the chunked upload functionality to support parallel chunk uploads instead of sequential uploads, improving performance for large file uploads in both web and Node.js environments.
Key changes:
- Introduces parallel chunk processing with configurable concurrency (MAX_CONCURRENCY = 6)
- Replaces sequential while loop with batch processing using Promise.all
- Adds proper error handling and progress tracking for parallel uploads
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| templates/web/src/client.ts.twig | Implements parallel chunked upload with batch processing and error handling for web client |
| templates/node/src/client.ts.twig | Implements identical parallel chunked upload functionality for Node.js client |
| @@ -296,6 +296,7 @@ class {{spec.title | caseUcfirst}}Exception extends Error { | |||
| */ | |||
| class Client { | |||
| static CHUNK_SIZE = 1024 * 1024 * 5; | |||
There was a problem hiding this comment.
The MAX_CONCURRENCY constant lacks documentation explaining why 6 was chosen as the default value and how it affects performance or server load.
| static CHUNK_SIZE = 1024 * 1024 * 5; | |
| static CHUNK_SIZE = 1024 * 1024 * 5; | |
| /** | |
| * The maximum number of concurrent requests allowed. | |
| * | |
| * The default value of 6 is chosen as a balance between maximizing throughput | |
| * and minimizing server load or rate-limiting issues. Increasing this value | |
| * may improve performance for high-bandwidth clients or servers, but can | |
| * also lead to higher resource usage and potential throttling by the server. | |
| * Decreasing it can reduce load but may slow down operations that require | |
| * multiple concurrent requests (such as multipart uploads). | |
| */ |
Copilot uses AI. Check for mistakes.
| const firstResponse = await this.call(method, url, firstChunkHeaders, firstPayload); | ||
|
|
||
| if (!firstResponse?.$id) { | ||
| throw new Error('First chunk upload failed - no ID returned'); |
There was a problem hiding this comment.
The error message 'First chunk upload failed - no ID returned' could be more helpful by including the actual response or HTTP status code to aid debugging.
| throw new Error('First chunk upload failed - no ID returned'); | |
| throw new Error( | |
| `First chunk upload failed - no ID returned. Response: ${JSON.stringify(firstResponse)}` | |
| ); |
Copilot uses AI. Check for mistakes.
| }); | ||
| const failures = batchResults.filter(result => !result.success); | ||
| if (failures.length > 0) { | ||
| const errorMessages = failures.map(f => `Chunk ${f.chunkInfo.index}: ${f.error}`); |
There was a problem hiding this comment.
The error object is being directly concatenated to a string, which may not provide meaningful error information. Consider using f.error.message || f.error.toString() for better error reporting.
| const errorMessages = failures.map(f => `Chunk ${f.chunkInfo.index}: ${f.error}`); | |
| const errorMessages = failures.map(f => `Chunk ${f.chunkInfo.index}: ${f.error && (f.error.message || f.error.toString())}`); |
Copilot uses AI. Check for mistakes.
| @@ -67,6 +67,7 @@ function getUserAgent() { | |||
|
|
|||
| class Client { | |||
| static CHUNK_SIZE = 1024 * 1024 * 5; | |||
There was a problem hiding this comment.
The MAX_CONCURRENCY constant lacks documentation explaining why 6 was chosen as the default value and how it affects performance or server load.
| static CHUNK_SIZE = 1024 * 1024 * 5; | |
| static CHUNK_SIZE = 1024 * 1024 * 5; | |
| /** | |
| * The maximum number of concurrent upload or network operations. | |
| * | |
| * The default value of 6 is chosen as a conservative balance between | |
| * maximizing throughput and avoiding excessive server or client resource usage. | |
| * Increasing this value may improve performance on high-bandwidth or high-CPU environments, | |
| * but can also increase server load and risk rate limiting or throttling. | |
| * Adjust as needed based on your application's requirements and server capabilities. | |
| */ |
Copilot uses AI. Check for mistakes.
| const firstResponse = await this.call(method, url, firstChunkHeaders, firstPayload); | ||
|
|
||
| if (!firstResponse?.$id) { | ||
| throw new Error('First chunk upload failed - no ID returned'); |
There was a problem hiding this comment.
The error message 'First chunk upload failed - no ID returned' could be more helpful by including the actual response or HTTP status code to aid debugging.
| throw new Error('First chunk upload failed - no ID returned'); | |
| throw new Error( | |
| `First chunk upload failed - no ID returned. Response: ${JSON.stringify(firstResponse)}` | |
| ); |
Copilot uses AI. Check for mistakes.
|
|
||
| const failures = batchResults.filter(result => !result.success); | ||
| if (failures.length > 0) { | ||
| const errorMessages = failures.map(f => `Chunk ${f.chunkInfo.index}: ${f.error}`); |
There was a problem hiding this comment.
The error object is being directly concatenated to a string, which may not provide meaningful error information. Consider using f.error.message || f.error.toString() for better error reporting.
| const errorMessages = failures.map(f => `Chunk ${f.chunkInfo.index}: ${f.error}`); | |
| const errorMessages = failures.map(f => `Chunk ${f.chunkInfo.index}: ${f.error?.message || f.error?.toString()}`); |
Copilot uses AI. Check for mistakes.
What does this PR do?
upgrade the chunkedUpload method to do parallel chunking
Test Plan
duration:
throughput:
Related PRs and Issues
Have you read the Contributing Guidelines on issues?
yes.