You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@opendal.apache.org by "Xuanwo (via GitHub)" <gi...@apache.org> on 2023/08/02 11:02:56 UTC
[GitHub] [incubator-opendal] Xuanwo opened a new pull request, #2758: rfc: Add Merge Append into write
Xuanwo opened a new pull request, #2758:
URL: https://github.com/apache/incubator-opendal/pull/2758
(no comment)
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] suyanhanx commented on a diff in pull request #2758: RFC-2758: Merge Append Into Write
Posted by "suyanhanx (via GitHub)" <gi...@apache.org>.
suyanhanx commented on code in PR #2758:
URL: https://github.com/apache/incubator-opendal/pull/2758#discussion_r1281783684
##########
core/src/docs/rfcs/0000_merge_append_into_write.md:
##########
@@ -0,0 +1,79 @@
+- Proposal Name: `merge_append_into_write`
+- Start Date: 2023-08-02
+- RFC PR: [apache/incubator-opendal#0000](https://github.com/apache/incubator-opendal/pull/0000)
+- Tracking Issue: [apache/incubator-opendal#0000](https://github.com/apache/incubator-opendal/issues/0000)
+
+# Summary
+
+Merge the `appender` API into `writer` by introducing a new `writer_with.append(true)` method to enable append mode.
+
+# Motivation
+
+Currently OpenDAL has separate `appender` and `writer` APIs:
+
+```rust
+let mut appender = op.appender_with("file.txt").await?;
+
+appender.append(bs).await?;
+appender.append(bs).await?;
+```
+
+This duplication forces users to learn two different APIs for writing data.
+
+By adding this change, we can:
+
+- Simpler API surface - users only need to learn one writing API.
+- Reduce code duplication between append and write implementations.
+- Atomic append semantics are handled internally in `writer`.
+- Reuse the `sink` api for both `overwrite` and `append` mode.
+
+# Guide-level explanation
+
+The new approach is to enable append mode on `writer`:
+
+```rust
+let mut writer = op.writer_with("file.txt").append(true).await?;
+
+writer.write(bs).await?; // appends to file
+writer.write(bs).await?; // appends again
+```
+
+Calling `writer_with.append(true)` will start the writer in append mode. Subsequent `write()` calls will append rather than overwrite.
+
+There is no longer a separate `appender` API.
+
+# Reference-level explanation
Review Comment:
Lots of work. Well, it's ok.
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] suyanhanx commented on a diff in pull request #2758: RFC-2758: Merge Append Into Write
Posted by "suyanhanx (via GitHub)" <gi...@apache.org>.
suyanhanx commented on code in PR #2758:
URL: https://github.com/apache/incubator-opendal/pull/2758#discussion_r1281766410
##########
core/src/docs/rfcs/0000_merge_append_into_write.md:
##########
@@ -0,0 +1,79 @@
+- Proposal Name: `merge_append_into_write`
+- Start Date: 2023-08-02
+- RFC PR: [apache/incubator-opendal#0000](https://github.com/apache/incubator-opendal/pull/0000)
+- Tracking Issue: [apache/incubator-opendal#0000](https://github.com/apache/incubator-opendal/issues/0000)
+
+# Summary
+
+Merge the `appender` API into `writer` by introducing a new `writer_with.append(true)` method to enable append mode.
+
+# Motivation
+
+Currently OpenDAL has separate `appender` and `writer` APIs:
+
+```rust
+let mut appender = op.appender_with("file.txt").await?;
+
+appender.append(bs).await?;
+appender.append(bs).await?;
+```
+
+This duplication forces users to learn two different APIs for writing data.
+
+By adding this change, we can:
+
+- Simpler API surface - users only need to learn one writing API.
+- Reduce code duplication between append and write implementations.
+- Atomic append semantics are handled internally in `writer`.
+- Reuse the `sink` api for both `overwrite` and `append` mode.
+
+# Guide-level explanation
+
+The new approach is to enable append mode on `writer`:
+
+```rust
+let mut writer = op.writer_with("file.txt").append(true).await?;
+
+writer.write(bs).await?; // appends to file
+writer.write(bs).await?; // appends again
+```
+
+Calling `writer_with.append(true)` will start the writer in append mode. Subsequent `write()` calls will append rather than overwrite.
+
+There is no longer a separate `appender` API.
+
+# Reference-level explanation
Review Comment:
Any idea about the appender in raw API?
##########
core/src/docs/rfcs/0000_merge_append_into_write.md:
##########
@@ -0,0 +1,79 @@
+- Proposal Name: `merge_append_into_write`
+- Start Date: 2023-08-02
+- RFC PR: [apache/incubator-opendal#0000](https://github.com/apache/incubator-opendal/pull/0000)
+- Tracking Issue: [apache/incubator-opendal#0000](https://github.com/apache/incubator-opendal/issues/0000)
+
+# Summary
+
+Merge the `appender` API into `writer` by introducing a new `writer_with.append(true)` method to enable append mode.
+
+# Motivation
+
+Currently OpenDAL has separate `appender` and `writer` APIs:
+
+```rust
+let mut appender = op.appender_with("file.txt").await?;
+
+appender.append(bs).await?;
+appender.append(bs).await?;
+```
+
+This duplication forces users to learn two different APIs for writing data.
+
+By adding this change, we can:
+
+- Simpler API surface - users only need to learn one writing API.
+- Reduce code duplication between append and write implementations.
+- Atomic append semantics are handled internally in `writer`.
+- Reuse the `sink` api for both `overwrite` and `append` mode.
Review Comment:
That's great.
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] Xuanwo commented on a diff in pull request #2758: RFC-2758: Merge Append Into Write
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #2758:
URL: https://github.com/apache/incubator-opendal/pull/2758#discussion_r1281782366
##########
core/src/docs/rfcs/0000_merge_append_into_write.md:
##########
@@ -0,0 +1,79 @@
+- Proposal Name: `merge_append_into_write`
+- Start Date: 2023-08-02
+- RFC PR: [apache/incubator-opendal#0000](https://github.com/apache/incubator-opendal/pull/0000)
+- Tracking Issue: [apache/incubator-opendal#0000](https://github.com/apache/incubator-opendal/issues/0000)
+
+# Summary
+
+Merge the `appender` API into `writer` by introducing a new `writer_with.append(true)` method to enable append mode.
+
+# Motivation
+
+Currently OpenDAL has separate `appender` and `writer` APIs:
+
+```rust
+let mut appender = op.appender_with("file.txt").await?;
+
+appender.append(bs).await?;
+appender.append(bs).await?;
+```
+
+This duplication forces users to learn two different APIs for writing data.
+
+By adding this change, we can:
+
+- Simpler API surface - users only need to learn one writing API.
+- Reduce code duplication between append and write implementations.
+- Atomic append semantics are handled internally in `writer`.
+- Reuse the `sink` api for both `overwrite` and `append` mode.
+
+# Guide-level explanation
+
+The new approach is to enable append mode on `writer`:
+
+```rust
+let mut writer = op.writer_with("file.txt").append(true).await?;
+
+writer.write(bs).await?; // appends to file
+writer.write(bs).await?; // appends again
+```
+
+Calling `writer_with.append(true)` will start the writer in append mode. Subsequent `write()` calls will append rather than overwrite.
+
+There is no longer a separate `appender` API.
+
+# Reference-level explanation
Review Comment:
> Any idea about the appender in raw API?
Also removed to make our trait simple.
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org