Xuanwo commented on code in PR #3898: URL: https://github.com/apache/incubator-opendal/pull/3898#discussion_r1439602398
########## core/src/docs/rfcs/3898_concurrent_writer.md: ########## @@ -0,0 +1,48 @@ +- Proposal Name: `concurrent_writer` +- Start Date: 2024-01-02 +- RFC PR: [apache/incubator-opendal#3898](https://github.com/apache/incubator-opendal/pull/3898) +- Tracking Issue: [apache/incubator-opendal#3899](https://github.com/apache/incubator-opendal/issues/3899) + +# Summary + +Add concurrent write in `MultipartUploadWriter`. + +# Motivation + +The [object_writer](./1420_object_writer.md) introduces the `ObjectWriter` multipart upload support. However, the multiple parts are currently uploaded serially without fully leveraging the potential for improved throughput through concurrent uploads. We should support the upload of multiple parts concurrently. + +# Guide-level explanation + +For users who want to concurrent writer, they will call the new API `concurrent`. And the default behavior remains unchanged, so users using `op.writer_with()` are not affected. The `concurrent` function will take a number as input, and this number will represent the maximum concurrent write task amount the writer can perform. + +```rust +op.writer_with(path).concurrent(8).await +``` + +# Reference-level explanation + +This feature will be implemented in the `MultipartUploadWriter`, which will utilize a `ConcurrentFutures<WriteTask>` as a task queue to store concurrent write tasks. Review Comment: Although not all services may support this feature, we should consider them all. As we design features for OpenDAL, our aim is to ensure consistent behavior across services. For examples: - What will happend if services support concurrent write? - How will they support this feature? - How will services with `MultipartUploadWriter` implement? - How will services with `RangeWriter` implement? - What will happened if services doesn't support this feature? - Will users got an error? - Or will we fallback to normal write? - Is it possible in the future? - For example, we know `fs` can't support concurrent write. But what if we use `write_at` instead? ########## core/src/docs/rfcs/3898_concurrent_writer.md: ########## @@ -0,0 +1,48 @@ +- Proposal Name: `concurrent_writer` +- Start Date: 2024-01-02 +- RFC PR: [apache/incubator-opendal#3898](https://github.com/apache/incubator-opendal/pull/3898) +- Tracking Issue: [apache/incubator-opendal#3899](https://github.com/apache/incubator-opendal/issues/3899) + +# Summary + +Add concurrent write in `MultipartUploadWriter`. + +# Motivation + +The [object_writer](./1420_object_writer.md) introduces the `ObjectWriter` multipart upload support. However, the multiple parts are currently uploaded serially without fully leveraging the potential for improved throughput through concurrent uploads. We should support the upload of multiple parts concurrently. Review Comment: The `ObjectWriter` is outdated and doesn't need to be mentioned here. How about including a demo of how data writing is currently performed? And explain where we can improve. ########## core/src/docs/rfcs/3898_concurrent_writer.md: ########## @@ -0,0 +1,48 @@ +- Proposal Name: `concurrent_writer` +- Start Date: 2024-01-02 +- RFC PR: [apache/incubator-opendal#3898](https://github.com/apache/incubator-opendal/pull/3898) +- Tracking Issue: [apache/incubator-opendal#3899](https://github.com/apache/incubator-opendal/issues/3899) + +# Summary + +Add concurrent write in `MultipartUploadWriter`. + +# Motivation + +The [object_writer](./1420_object_writer.md) introduces the `ObjectWriter` multipart upload support. However, the multiple parts are currently uploaded serially without fully leveraging the potential for improved throughput through concurrent uploads. We should support the upload of multiple parts concurrently. + +# Guide-level explanation + +For users who want to concurrent writer, they will call the new API `concurrent`. And the default behavior remains unchanged, so users using `op.writer_with()` are not affected. The `concurrent` function will take a number as input, and this number will represent the maximum concurrent write task amount the writer can perform. + +```rust +op.writer_with(path).concurrent(8).await +``` + +# Reference-level explanation + +This feature will be implemented in the `MultipartUploadWriter`, which will utilize a `ConcurrentFutures<WriteTask>` as a task queue to store concurrent write tasks. + +A `concurrent` field of type `usize` will be introduced to `OpWrite`. If `concurrent` is set to 0 or 1, it functions with default behavior. However, if concurrent is set to number larger than 1, it denotes the maximum concurrent write task amount that the `MultipartUploadWriter` can utilize. + +When the upper layer invokes `poll_write`, the `MultipartUploadWriter` pushes `concurrent` upload parts to the task queue (`ConcurrentFutures<WriteTask>`) if there are available slots. If the task queue is full, the `MultipartUploadWriter` waits for the first task to yield results. Review Comment: Let's clarify the behavior. So, when the `concurrent` value is set, will users be able to write more data without waiting for the existing part to finish? ########## core/src/docs/rfcs/3898_concurrent_writer.md: ########## @@ -0,0 +1,48 @@ +- Proposal Name: `concurrent_writer` +- Start Date: 2024-01-02 +- RFC PR: [apache/incubator-opendal#3898](https://github.com/apache/incubator-opendal/pull/3898) +- Tracking Issue: [apache/incubator-opendal#3899](https://github.com/apache/incubator-opendal/issues/3899) + +# Summary + +Add concurrent write in `MultipartUploadWriter`. + +# Motivation + +The [object_writer](./1420_object_writer.md) introduces the `ObjectWriter` multipart upload support. However, the multiple parts are currently uploaded serially without fully leveraging the potential for improved throughput through concurrent uploads. We should support the upload of multiple parts concurrently. + +# Guide-level explanation + +For users who want to concurrent writer, they will call the new API `concurrent`. And the default behavior remains unchanged, so users using `op.writer_with()` are not affected. The `concurrent` function will take a number as input, and this number will represent the maximum concurrent write task amount the writer can perform. Review Comment: Please clarify the usage. For instance, how does setting `concurrent` to `8` change its behavior? What should users expect? How will this feature interact with others, such as `buffer`? ########## core/src/docs/rfcs/3898_concurrent_writer.md: ########## @@ -0,0 +1,48 @@ +- Proposal Name: `concurrent_writer` +- Start Date: 2024-01-02 +- RFC PR: [apache/incubator-opendal#3898](https://github.com/apache/incubator-opendal/pull/3898) +- Tracking Issue: [apache/incubator-opendal#3899](https://github.com/apache/incubator-opendal/issues/3899) + +# Summary + +Add concurrent write in `MultipartUploadWriter`. Review Comment: I'm not in favor of adding features to internal types. OpenDAL maintains clear boundaries between its public and raw APIs. We should exclusively expose the public API to users, who shouldn't need to be aware of something called `MultipartUploadWriter`. ########## core/src/docs/rfcs/3898_concurrent_writer.md: ########## @@ -0,0 +1,48 @@ +- Proposal Name: `concurrent_writer` +- Start Date: 2024-01-02 +- RFC PR: [apache/incubator-opendal#3898](https://github.com/apache/incubator-opendal/pull/3898) +- Tracking Issue: [apache/incubator-opendal#3899](https://github.com/apache/incubator-opendal/issues/3899) + +# Summary + +Add concurrent write in `MultipartUploadWriter`. + +# Motivation + +The [object_writer](./1420_object_writer.md) introduces the `ObjectWriter` multipart upload support. However, the multiple parts are currently uploaded serially without fully leveraging the potential for improved throughput through concurrent uploads. We should support the upload of multiple parts concurrently. + +# Guide-level explanation + +For users who want to concurrent writer, they will call the new API `concurrent`. And the default behavior remains unchanged, so users using `op.writer_with()` are not affected. The `concurrent` function will take a number as input, and this number will represent the maximum concurrent write task amount the writer can perform. + +```rust +op.writer_with(path).concurrent(8).await +``` + +# Reference-level explanation + +This feature will be implemented in the `MultipartUploadWriter`, which will utilize a `ConcurrentFutures<WriteTask>` as a task queue to store concurrent write tasks. + +A `concurrent` field of type `usize` will be introduced to `OpWrite`. If `concurrent` is set to 0 or 1, it functions with default behavior. However, if concurrent is set to number larger than 1, it denotes the maximum concurrent write task amount that the `MultipartUploadWriter` can utilize. + +When the upper layer invokes `poll_write`, the `MultipartUploadWriter` pushes `concurrent` upload parts to the task queue (`ConcurrentFutures<WriteTask>`) if there are available slots. If the task queue is full, the `MultipartUploadWriter` waits for the first task to yield results. + +# Drawbacks + +None Review Comment: More memory usage, More concurrent connections ########## core/src/docs/rfcs/3898_concurrent_writer.md: ########## @@ -0,0 +1,48 @@ +- Proposal Name: `concurrent_writer` +- Start Date: 2024-01-02 +- RFC PR: [apache/incubator-opendal#3898](https://github.com/apache/incubator-opendal/pull/3898) +- Tracking Issue: [apache/incubator-opendal#3899](https://github.com/apache/incubator-opendal/issues/3899) + +# Summary + +Add concurrent write in `MultipartUploadWriter`. + +# Motivation + +The [object_writer](./1420_object_writer.md) introduces the `ObjectWriter` multipart upload support. However, the multiple parts are currently uploaded serially without fully leveraging the potential for improved throughput through concurrent uploads. We should support the upload of multiple parts concurrently. + +# Guide-level explanation + +For users who want to concurrent writer, they will call the new API `concurrent`. And the default behavior remains unchanged, so users using `op.writer_with()` are not affected. The `concurrent` function will take a number as input, and this number will represent the maximum concurrent write task amount the writer can perform. + +```rust +op.writer_with(path).concurrent(8).await +``` + +# Reference-level explanation + +This feature will be implemented in the `MultipartUploadWriter`, which will utilize a `ConcurrentFutures<WriteTask>` as a task queue to store concurrent write tasks. + +A `concurrent` field of type `usize` will be introduced to `OpWrite`. If `concurrent` is set to 0 or 1, it functions with default behavior. However, if concurrent is set to number larger than 1, it denotes the maximum concurrent write task amount that the `MultipartUploadWriter` can utilize. + +When the upper layer invokes `poll_write`, the `MultipartUploadWriter` pushes `concurrent` upload parts to the task queue (`ConcurrentFutures<WriteTask>`) if there are available slots. If the task queue is full, the `MultipartUploadWriter` waits for the first task to yield results. + +# Drawbacks + +None + +# Rationale and alternatives + +None + +# Prior art + +None + +# Unresolved questions + +None + +# Future possibilities + +None Review Comment: Maybe we can add `write_at` for `fs` here? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
