You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@opendal.apache.org by "ShadowySpirits (via GitHub)" <gi...@apache.org> on 2023/04/26 11:58:44 UTC
[GitHub] [incubator-opendal] ShadowySpirits opened a new pull request, #2133: RFC: Introduce Append API
ShadowySpirits opened a new pull request, #2133:
URL: https://github.com/apache/incubator-opendal/pull/2133
### Discussed in https://github.com/apache/incubator-opendal/discussions/2115
<div type='discussions-op-text'>
<sup>Originally posted by **ShadowySpirits** April 25, 2023</sup>
OpenDAL provides append-alike operation, which is based on multipart API. But this approach has some limitations:
- Data could be lost and not readable before w.close() returned Ok(())
- File can't be append again after w.close() returned Ok(())
Currently, there are not many docs about this. To ensure proper usage, it is important to inform users about these risks. Furthermore, for services that support append API natively, using append API instead of multipart API is a better choice.
I propose to separate the write and append operations. The files created by the write operation can not be modified. And the files created by the append operation can be appended via multipart API or append API for services that natively support append, such as [Azure blob](https://learn.microsoft.com/en-us/rest/api/storageservices/append-block?tabs=azure-ad) and [Alibaba cloud OSS](https://www.alibabacloud.com/help/en/object-storage-service/latest/appendobject).</div>
--
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] ShadowySpirits commented on a diff in pull request #2133: RFC-2133: Introduce Append API
Posted by "ShadowySpirits (via GitHub)" <gi...@apache.org>.
ShadowySpirits commented on code in PR #2133:
URL: https://github.com/apache/incubator-opendal/pull/2133#discussion_r1179027920
##########
core/src/docs/rfcs/2133_append_api.md:
##########
@@ -0,0 +1,106 @@
+- Proposal Name: `append_api`
+- Start Date: 2023-04-26
+- RFC PR: [apache/incubator-opendal#2133](https://github.com/apache/incubator-opendal/pull/2133)
+- Tracking Issue:
+
+# Summary
+
+Introduce append operations for OpenDAL which allow users to add data to a file. And write operation will be not reentrant.
+
+# Motivation
+
+OpenDAL has the write operation used to create a file and append data to it. This is implemented based on multipart API. However, current approach has some limitations:
+
+- Data could be lost and not readable before w.close() returned Ok(())
+- File can't be appended again after w.close() returned Ok(())
+
+To address these issues, I propose adding an append operation and modifying the write operation to verify consistency between content length and data size. The revised write operation will not be reentrant; however, users can create an appender that provides a reentrant append operation.
+
+# Guide-level explanation
+
+The files created by the write operation can not be modified, only overwritten.
+
+```rust
+async fn write_test(op: Operation) -> Result<()> {
+ let bs = read_from_file();
+ op.write("path_to_file", bs).await?;
+
+ // The following code will report an error
+ op.write("path_to_file", bs).await?;
+
+ // Unless specified can overwrite
+ let option = OpWrite::default().with_allow_overwrite(true);
+ // content length must be the same as the data size
+ option.with_content_length(bs.len() as u64);
+ op.write_with("path_to_file", option, bs).await?;
+}
+```
+
+The files created by the append operation can be appended via multipart API or append API.
Review Comment:
Ok, the proposal's goal will be to add the append API while leaving the write API untouched.
--
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 #2133: RFC-2133: Introduce Append API
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #2133:
URL: https://github.com/apache/incubator-opendal/pull/2133#discussion_r1178994881
##########
core/src/docs/rfcs/2133_append_api.md:
##########
@@ -0,0 +1,106 @@
+- Proposal Name: `append_api`
+- Start Date: 2023-04-26
+- RFC PR: [apache/incubator-opendal#2133](https://github.com/apache/incubator-opendal/pull/2133)
+- Tracking Issue:
+
+# Summary
+
+Introduce append operations for OpenDAL which allow users to add data to a file. And write operation will be not reentrant.
+
+# Motivation
+
+OpenDAL has the write operation used to create a file and append data to it. This is implemented based on multipart API. However, current approach has some limitations:
+
+- Data could be lost and not readable before w.close() returned Ok(())
+- File can't be appended again after w.close() returned Ok(())
+
+To address these issues, I propose adding an append operation and modifying the write operation to verify consistency between content length and data size. The revised write operation will not be reentrant; however, users can create an appender that provides a reentrant append operation.
+
+# Guide-level explanation
+
+The files created by the write operation can not be modified, only overwritten.
+
+```rust
+async fn write_test(op: Operation) -> Result<()> {
+ let bs = read_from_file();
+ op.write("path_to_file", bs).await?;
+
+ // The following code will report an error
+ op.write("path_to_file", bs).await?;
+
+ // Unless specified can overwrite
+ let option = OpWrite::default().with_allow_overwrite(true);
+ // content length must be the same as the data size
+ option.with_content_length(bs.len() as u64);
+ op.write_with("path_to_file", option, bs).await?;
+}
+```
+
+The files created by the append operation can be appended via multipart API or append API.
Review Comment:
multipart API can't be used as `append` because:
- Users can't append on an exisiting path.
- Data is not readable during multipart upload.
##########
core/src/docs/rfcs/2133_append_api.md:
##########
@@ -0,0 +1,106 @@
+- Proposal Name: `append_api`
+- Start Date: 2023-04-26
+- RFC PR: [apache/incubator-opendal#2133](https://github.com/apache/incubator-opendal/pull/2133)
+- Tracking Issue:
+
+# Summary
+
+Introduce append operations for OpenDAL which allow users to add data to a file. And write operation will be not reentrant.
+
+# Motivation
+
+OpenDAL has the write operation used to create a file and append data to it. This is implemented based on multipart API. However, current approach has some limitations:
+
+- Data could be lost and not readable before w.close() returned Ok(())
+- File can't be appended again after w.close() returned Ok(())
+
+To address these issues, I propose adding an append operation and modifying the write operation to verify consistency between content length and data size. The revised write operation will not be reentrant; however, users can create an appender that provides a reentrant append operation.
+
+# Guide-level explanation
+
+The files created by the write operation can not be modified, only overwritten.
+
+```rust
+async fn write_test(op: Operation) -> Result<()> {
+ let bs = read_from_file();
+ op.write("path_to_file", bs).await?;
+
+ // The following code will report an error
+ op.write("path_to_file", bs).await?;
+
+ // Unless specified can overwrite
+ let option = OpWrite::default().with_allow_overwrite(true);
+ // content length must be the same as the data size
+ option.with_content_length(bs.len() as u64);
+ op.write_with("path_to_file", option, bs).await?;
+}
+```
Review Comment:
This part should be moved to `Motivation` instead.
##########
core/src/docs/rfcs/2133_append_api.md:
##########
@@ -0,0 +1,106 @@
+- Proposal Name: `append_api`
+- Start Date: 2023-04-26
+- RFC PR: [apache/incubator-opendal#2133](https://github.com/apache/incubator-opendal/pull/2133)
+- Tracking Issue:
+
+# Summary
+
+Introduce append operations for OpenDAL which allow users to add data to a file. And write operation will be not reentrant.
+
+# Motivation
+
+OpenDAL has the write operation used to create a file and append data to it. This is implemented based on multipart API. However, current approach has some limitations:
+
+- Data could be lost and not readable before w.close() returned Ok(())
+- File can't be appended again after w.close() returned Ok(())
+
+To address these issues, I propose adding an append operation and modifying the write operation to verify consistency between content length and data size. The revised write operation will not be reentrant; however, users can create an appender that provides a reentrant append operation.
+
+# Guide-level explanation
+
+The files created by the write operation can not be modified, only overwritten.
+
+```rust
+async fn write_test(op: Operation) -> Result<()> {
+ let bs = read_from_file();
+ op.write("path_to_file", bs).await?;
+
+ // The following code will report an error
+ op.write("path_to_file", bs).await?;
+
+ // Unless specified can overwrite
+ let option = OpWrite::default().with_allow_overwrite(true);
+ // content length must be the same as the data size
+ option.with_content_length(bs.len() as u64);
+ op.write_with("path_to_file", option, bs).await?;
+}
+```
+
+The files created by the append operation can be appended via multipart API or append API.
+
+```rust
+async fn append_test(op: Operation) -> Result<()> {
+ // create writer
+ let appender = op.appender("path_to_file").await?;
+
+ let bs = read_from_file();
+ appender.append(bs).await?;
+
+ let bs = read_from_another_file();
+ appender.append(bs).await?;
+
+ // close the file
+ // for multipart API, complete the upload and prevent writing after close
+ // for native append API, this is a no-op
+ appender.close().await?;
+
+ // abort the operation and delete all uploaded data
+ appender.abort().await?;
Review Comment:
Append doesn't have `abort`, users have to `delete` it.
##########
core/src/docs/rfcs/2133_append_api.md:
##########
@@ -0,0 +1,106 @@
+- Proposal Name: `append_api`
+- Start Date: 2023-04-26
+- RFC PR: [apache/incubator-opendal#2133](https://github.com/apache/incubator-opendal/pull/2133)
+- Tracking Issue:
+
+# Summary
+
+Introduce append operations for OpenDAL which allow users to add data to a file. And write operation will be not reentrant.
+
+# Motivation
+
+OpenDAL has the write operation used to create a file and append data to it. This is implemented based on multipart API. However, current approach has some limitations:
+
+- Data could be lost and not readable before w.close() returned Ok(())
+- File can't be appended again after w.close() returned Ok(())
+
+To address these issues, I propose adding an append operation and modifying the write operation to verify consistency between content length and data size. The revised write operation will not be reentrant; however, users can create an appender that provides a reentrant append operation.
+
+# Guide-level explanation
+
+The files created by the write operation can not be modified, only overwritten.
+
+```rust
+async fn write_test(op: Operation) -> Result<()> {
+ let bs = read_from_file();
+ op.write("path_to_file", bs).await?;
+
+ // The following code will report an error
+ op.write("path_to_file", bs).await?;
+
+ // Unless specified can overwrite
+ let option = OpWrite::default().with_allow_overwrite(true);
+ // content length must be the same as the data size
+ option.with_content_length(bs.len() as u64);
+ op.write_with("path_to_file", option, bs).await?;
+}
+```
+
+The files created by the append operation can be appended via multipart API or append API.
+
+```rust
+async fn append_test(op: Operation) -> Result<()> {
+ // create writer
+ let appender = op.appender("path_to_file").await?;
+
+ let bs = read_from_file();
+ appender.append(bs).await?;
+
+ let bs = read_from_another_file();
+ appender.append(bs).await?;
+
+ // close the file
+ // for multipart API, complete the upload and prevent writing after close
+ // for native append API, this is a no-op
+ appender.close().await?;
+
+ // abort the operation and delete all uploaded data
+ appender.abort().await?;
+}
+```
+
+# Reference-level explanation
+
+To implement this feature, we need to add a new API `append` into `oio::Append`. And remove useless function in `oio::Write`.
Review Comment:
No, this understanding is wrong.
We should not touch exsting APIs of `oio::Write`. They are different APIs.
##########
core/src/docs/rfcs/2133_append_api.md:
##########
@@ -0,0 +1,106 @@
+- Proposal Name: `append_api`
+- Start Date: 2023-04-26
+- RFC PR: [apache/incubator-opendal#2133](https://github.com/apache/incubator-opendal/pull/2133)
+- Tracking Issue:
+
+# Summary
+
+Introduce append operations for OpenDAL which allow users to add data to a file. And write operation will be not reentrant.
+
+# Motivation
+
+OpenDAL has the write operation used to create a file and append data to it. This is implemented based on multipart API. However, current approach has some limitations:
+
+- Data could be lost and not readable before w.close() returned Ok(())
+- File can't be appended again after w.close() returned Ok(())
+
+To address these issues, I propose adding an append operation and modifying the write operation to verify consistency between content length and data size. The revised write operation will not be reentrant; however, users can create an appender that provides a reentrant append operation.
+
+# Guide-level explanation
+
+The files created by the write operation can not be modified, only overwritten.
+
+```rust
+async fn write_test(op: Operation) -> Result<()> {
+ let bs = read_from_file();
+ op.write("path_to_file", bs).await?;
+
+ // The following code will report an error
+ op.write("path_to_file", bs).await?;
+
+ // Unless specified can overwrite
+ let option = OpWrite::default().with_allow_overwrite(true);
+ // content length must be the same as the data size
+ option.with_content_length(bs.len() as u64);
+ op.write_with("path_to_file", option, bs).await?;
+}
+```
+
+The files created by the append operation can be appended via multipart API or append API.
+
+```rust
+async fn append_test(op: Operation) -> Result<()> {
+ // create writer
+ let appender = op.appender("path_to_file").await?;
Review Comment:
I expect to use `op.append()`
##########
core/src/docs/rfcs/2133_append_api.md:
##########
@@ -0,0 +1,106 @@
+- Proposal Name: `append_api`
+- Start Date: 2023-04-26
+- RFC PR: [apache/incubator-opendal#2133](https://github.com/apache/incubator-opendal/pull/2133)
+- Tracking Issue:
+
+# Summary
+
+Introduce append operations for OpenDAL which allow users to add data to a file. And write operation will be not reentrant.
+
+# Motivation
+
+OpenDAL has the write operation used to create a file and append data to it. This is implemented based on multipart API. However, current approach has some limitations:
+
+- Data could be lost and not readable before w.close() returned Ok(())
+- File can't be appended again after w.close() returned Ok(())
+
+To address these issues, I propose adding an append operation and modifying the write operation to verify consistency between content length and data size. The revised write operation will not be reentrant; however, users can create an appender that provides a reentrant append operation.
+
+# Guide-level explanation
+
+The files created by the write operation can not be modified, only overwritten.
+
+```rust
+async fn write_test(op: Operation) -> Result<()> {
+ let bs = read_from_file();
+ op.write("path_to_file", bs).await?;
+
+ // The following code will report an error
+ op.write("path_to_file", bs).await?;
+
+ // Unless specified can overwrite
+ let option = OpWrite::default().with_allow_overwrite(true);
+ // content length must be the same as the data size
+ option.with_content_length(bs.len() as u64);
+ op.write_with("path_to_file", option, bs).await?;
+}
+```
+
+The files created by the append operation can be appended via multipart API or append API.
+
+```rust
+async fn append_test(op: Operation) -> Result<()> {
+ // create writer
+ let appender = op.appender("path_to_file").await?;
+
+ let bs = read_from_file();
+ appender.append(bs).await?;
+
+ let bs = read_from_another_file();
+ appender.append(bs).await?;
+
+ // close the file
+ // for multipart API, complete the upload and prevent writing after close
Review Comment:
This violates the semantics of `append` so we can' use `multipart` to implement `append`.
##########
core/src/docs/rfcs/2133_append_api.md:
##########
@@ -0,0 +1,106 @@
+- Proposal Name: `append_api`
+- Start Date: 2023-04-26
+- RFC PR: [apache/incubator-opendal#2133](https://github.com/apache/incubator-opendal/pull/2133)
+- Tracking Issue:
+
+# Summary
+
+Introduce append operations for OpenDAL which allow users to add data to a file. And write operation will be not reentrant.
+
+# Motivation
+
+OpenDAL has the write operation used to create a file and append data to it. This is implemented based on multipart API. However, current approach has some limitations:
+
+- Data could be lost and not readable before w.close() returned Ok(())
+- File can't be appended again after w.close() returned Ok(())
+
+To address these issues, I propose adding an append operation and modifying the write operation to verify consistency between content length and data size. The revised write operation will not be reentrant; however, users can create an appender that provides a reentrant append operation.
+
+# Guide-level explanation
+
+The files created by the write operation can not be modified, only overwritten.
+
+```rust
+async fn write_test(op: Operation) -> Result<()> {
+ let bs = read_from_file();
+ op.write("path_to_file", bs).await?;
+
+ // The following code will report an error
+ op.write("path_to_file", bs).await?;
+
+ // Unless specified can overwrite
+ let option = OpWrite::default().with_allow_overwrite(true);
+ // content length must be the same as the data size
+ option.with_content_length(bs.len() as u64);
+ op.write_with("path_to_file", option, bs).await?;
+}
+```
+
+The files created by the append operation can be appended via multipart API or append API.
+
+```rust
+async fn append_test(op: Operation) -> Result<()> {
+ // create writer
+ let appender = op.appender("path_to_file").await?;
+
+ let bs = read_from_file();
+ appender.append(bs).await?;
+
+ let bs = read_from_another_file();
+ appender.append(bs).await?;
+
+ // close the file
+ // for multipart API, complete the upload and prevent writing after close
+ // for native append API, this is a no-op
+ appender.close().await?;
+
+ // abort the operation and delete all uploaded data
+ appender.abort().await?;
+}
+```
+
+# Reference-level explanation
+
+To implement this feature, we need to add a new API `append` into `oio::Append`. And remove useless function in `oio::Write`.
+
+```rust
+#[async_trait]
+pub trait Append: Unpin + Send + Sync {
+ /// Append data to the end of file.
+ /// Users will call `append` multiple times. Please make sure `append` is safe to re-enter.
+ async fn append(&mut self, bs: Bytes) -> Result<()>;
+
+ /// Abort the pending writer.
+ async fn abort(&mut self) -> Result<()>;
+
+ /// Close the writer and make sure all data has been flushed.
+ async fn close(&mut self) -> Result<()>;
+}
+
+#[async_trait]
+pub trait Write: Unpin + Send + Sync {
Review Comment:
Maybe we don't need to mention `Write` here? Seems confusing.
##########
core/src/docs/rfcs/2133_append_api.md:
##########
@@ -0,0 +1,106 @@
+- Proposal Name: `append_api`
+- Start Date: 2023-04-26
+- RFC PR: [apache/incubator-opendal#2133](https://github.com/apache/incubator-opendal/pull/2133)
+- Tracking Issue:
+
+# Summary
+
+Introduce append operations for OpenDAL which allow users to add data to a file. And write operation will be not reentrant.
+
+# Motivation
+
+OpenDAL has the write operation used to create a file and append data to it. This is implemented based on multipart API. However, current approach has some limitations:
+
+- Data could be lost and not readable before w.close() returned Ok(())
+- File can't be appended again after w.close() returned Ok(())
+
+To address these issues, I propose adding an append operation and modifying the write operation to verify consistency between content length and data size. The revised write operation will not be reentrant; however, users can create an appender that provides a reentrant append operation.
+
+# Guide-level explanation
+
+The files created by the write operation can not be modified, only overwritten.
+
+```rust
+async fn write_test(op: Operation) -> Result<()> {
+ let bs = read_from_file();
+ op.write("path_to_file", bs).await?;
+
+ // The following code will report an error
+ op.write("path_to_file", bs).await?;
+
+ // Unless specified can overwrite
+ let option = OpWrite::default().with_allow_overwrite(true);
+ // content length must be the same as the data size
+ option.with_content_length(bs.len() as u64);
+ op.write_with("path_to_file", option, bs).await?;
+}
+```
+
+The files created by the append operation can be appended via multipart API or append API.
+
+```rust
+async fn append_test(op: Operation) -> Result<()> {
+ // create writer
+ let appender = op.appender("path_to_file").await?;
+
+ let bs = read_from_file();
+ appender.append(bs).await?;
+
+ let bs = read_from_another_file();
+ appender.append(bs).await?;
+
+ // close the file
+ // for multipart API, complete the upload and prevent writing after close
+ // for native append API, this is a no-op
+ appender.close().await?;
+
+ // abort the operation and delete all uploaded data
+ appender.abort().await?;
+}
+```
+
+# Reference-level explanation
+
+To implement this feature, we need to add a new API `append` into `oio::Append`. And remove useless function in `oio::Write`.
+
+```rust
+#[async_trait]
+pub trait Append: Unpin + Send + Sync {
+ /// Append data to the end of file.
+ /// Users will call `append` multiple times. Please make sure `append` is safe to re-enter.
+ async fn append(&mut self, bs: Bytes) -> Result<()>;
+
+ /// Abort the pending writer.
+ async fn abort(&mut self) -> Result<()>;
Review Comment:
ditto
--
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] ShadowySpirits commented on a diff in pull request #2133: RFC-2133: Introduce Append API
Posted by "ShadowySpirits (via GitHub)" <gi...@apache.org>.
ShadowySpirits commented on code in PR #2133:
URL: https://github.com/apache/incubator-opendal/pull/2133#discussion_r1177811557
##########
core/src/docs/rfcs/2133_introduce_append_api.md:
##########
@@ -0,0 +1,100 @@
+- Proposal Name: `introduce_append_api`
+- Start Date: 2023-04-26
+- RFC PR: https://github.com/apache/incubator-opendal/pull/2133
+- Tracking Issue:
+
+# Summary
+
+Separate append operations from the write operation.
+
+# Motivation
+
+OpenDAL has the write operation used to create a file and append data to it. This is implemented based on multipart API. However, current approach has some limitations:
+
+- Data could be lost and not readable before w.close() returned Ok(())
+- File can't be appended again after w.close() returned Ok(())
+
+To address these issues, I propose separating the write and append operations. The new write operation will not be reentrant, but users can create a writer that provides a reentrant append operation.
+
+# Guide-level explanation
+
+The files created by the write operation can not be modified, only overwritten.
+
+```rust
+async fn write_test(op: Operation) -> Result<()> {
+ let bs = read_from_file();
+ op.write("path_to_file", bs).await?;
+
+ // The following code will report an error
+ op.write("path_to_file", bs).await?;
+
+ // Unless specified can overwrite
+ let option = OpWrite::default().with_allow_overwrite(true);
+ op.write_with("path_to_file", option, bs).await?;
+}
+```
+
+The files created by the append operation can be appended via multipart API or append API.
+
+```rust
+async fn append_test(op: Operation) -> Result<()> {
+ // create writer
+ let writer = op.writer("path_to_file").await?;
+
+ let bs = read_from_file();
+ writer.append(bs).await?;
+
+ let bs = read_from_another_file();
+ writer.append(bs).await?;
+
+ // close the file
+ // for multipart API, complete the upload and prevent writing after close
+ // for native append API, this is a no-op
+ writer.close().await?;
+
+ // abort the operation and delete all uploaded data
+ writer.abort().await?;
+}
+```
+
+# Reference-level explanation
+
+To implement this feature, we need to add a new API `append` into `oio::Writer`. And change the `oio::Writer::write` to be not reentrant.
+
+```rust
+#[async_trait]
+pub trait Write: Unpin + Send + Sync {
+ /// Write given into writer. This operation will overwrite the file if it is supported.
+ async fn write(&mut self, bs: Bytes) -> Result<()>;
+
+ /// Append data to the end of file.
+ /// Users will call `append` multiple times. Please make sure `append` is safe to re-enter.
+ async fn append(&mut self, bs: Bytes) -> Result<()>;
Review Comment:
IMO, this trait is an internal API. The interface exposed to the user is described in the `Guide-level explanation` section. Please correct me if my understanding is wrong.
--
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 #2133: RFC-2133: Introduce Append API
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #2133:
URL: https://github.com/apache/incubator-opendal/pull/2133#discussion_r1177791713
##########
core/src/docs/rfcs/2133_introduce_append_api.md:
##########
@@ -0,0 +1,100 @@
+- Proposal Name: `introduce_append_api`
+- Start Date: 2023-04-26
+- RFC PR: https://github.com/apache/incubator-opendal/pull/2133
+- Tracking Issue:
+
+# Summary
+
+Separate append operations from the write operation.
+
+# Motivation
+
+OpenDAL has the write operation used to create a file and append data to it. This is implemented based on multipart API. However, current approach has some limitations:
+
+- Data could be lost and not readable before w.close() returned Ok(())
+- File can't be appended again after w.close() returned Ok(())
+
+To address these issues, I propose separating the write and append operations. The new write operation will not be reentrant, but users can create a writer that provides a reentrant append operation.
+
+# Guide-level explanation
+
+The files created by the write operation can not be modified, only overwritten.
+
+```rust
+async fn write_test(op: Operation) -> Result<()> {
+ let bs = read_from_file();
+ op.write("path_to_file", bs).await?;
+
+ // The following code will report an error
+ op.write("path_to_file", bs).await?;
+
+ // Unless specified can overwrite
+ let option = OpWrite::default().with_allow_overwrite(true);
+ op.write_with("path_to_file", option, bs).await?;
+}
+```
+
+The files created by the append operation can be appended via multipart API or append API.
+
+```rust
+async fn append_test(op: Operation) -> Result<()> {
+ // create writer
+ let writer = op.writer("path_to_file").await?;
+
+ let bs = read_from_file();
+ writer.append(bs).await?;
+
+ let bs = read_from_another_file();
+ writer.append(bs).await?;
+
+ // close the file
+ // for multipart API, complete the upload and prevent writing after close
+ // for native append API, this is a no-op
+ writer.close().await?;
+
+ // abort the operation and delete all uploaded data
+ writer.abort().await?;
+}
+```
+
+# Reference-level explanation
+
+To implement this feature, we need to add a new API `append` into `oio::Writer`. And change the `oio::Writer::write` to be not reentrant.
+
+```rust
+#[async_trait]
+pub trait Write: Unpin + Send + Sync {
+ /// Write given into writer. This operation will overwrite the file if it is supported.
+ async fn write(&mut self, bs: Bytes) -> Result<()>;
+
+ /// Append data to the end of file.
+ /// Users will call `append` multiple times. Please make sure `append` is safe to re-enter.
+ async fn append(&mut self, bs: Bytes) -> Result<()>;
Review Comment:
This design could be confusing. Users could call `write`, `append`, `write` which may behavior in wrong way.
I prefer to add a new trait called `Append`:
```rust
#[async_trait]
pub trait Append: Unpin + Send + Sync {
async fn append(&mut self, bs: Bytes) -> Result<()>;
async fn close(&mut self) -> Result<()>;
}
```
Our user will use our API like the following:
```rust
let ap = op.append("path/to.file").await?;
ap.append(bs).await?;
ap.append(bs).await?;
ap.close().await?;
```
This is no `abort` for `append` because data will be available for reading after `append` returned `Ok`. Users need to call `delete` to remove a file.
--
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 #2133: RFC-2133: Introduce Append API
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #2133:
URL: https://github.com/apache/incubator-opendal/pull/2133#discussion_r1177794351
##########
core/src/docs/rfcs/2133_introduce_append_api.md:
##########
@@ -0,0 +1,100 @@
+- Proposal Name: `introduce_append_api`
+- Start Date: 2023-04-26
+- RFC PR: https://github.com/apache/incubator-opendal/pull/2133
Review Comment:
Please keep our template.
##########
core/src/docs/rfcs/2133_introduce_append_api.md:
##########
@@ -0,0 +1,100 @@
+- Proposal Name: `introduce_append_api`
Review Comment:
How about using `append_api` directly?
##########
core/src/docs/rfcs/2133_introduce_append_api.md:
##########
@@ -0,0 +1,100 @@
+- Proposal Name: `introduce_append_api`
+- Start Date: 2023-04-26
+- RFC PR: https://github.com/apache/incubator-opendal/pull/2133
+- Tracking Issue:
+
+# Summary
+
+Separate append operations from the write operation.
Review Comment:
This is an addition instead of a `seperate`. `write` has different sementic with `append`.
--
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] ShadowySpirits commented on a diff in pull request #2133: RFC-2133: Introduce Append API
Posted by "ShadowySpirits (via GitHub)" <gi...@apache.org>.
ShadowySpirits commented on code in PR #2133:
URL: https://github.com/apache/incubator-opendal/pull/2133#discussion_r1179005679
##########
core/src/docs/rfcs/2133_append_api.md:
##########
@@ -0,0 +1,106 @@
+- Proposal Name: `append_api`
+- Start Date: 2023-04-26
+- RFC PR: [apache/incubator-opendal#2133](https://github.com/apache/incubator-opendal/pull/2133)
+- Tracking Issue:
+
+# Summary
+
+Introduce append operations for OpenDAL which allow users to add data to a file. And write operation will be not reentrant.
+
+# Motivation
+
+OpenDAL has the write operation used to create a file and append data to it. This is implemented based on multipart API. However, current approach has some limitations:
+
+- Data could be lost and not readable before w.close() returned Ok(())
+- File can't be appended again after w.close() returned Ok(())
+
+To address these issues, I propose adding an append operation and modifying the write operation to verify consistency between content length and data size. The revised write operation will not be reentrant; however, users can create an appender that provides a reentrant append operation.
+
+# Guide-level explanation
+
+The files created by the write operation can not be modified, only overwritten.
+
+```rust
+async fn write_test(op: Operation) -> Result<()> {
+ let bs = read_from_file();
+ op.write("path_to_file", bs).await?;
+
+ // The following code will report an error
+ op.write("path_to_file", bs).await?;
+
+ // Unless specified can overwrite
+ let option = OpWrite::default().with_allow_overwrite(true);
+ // content length must be the same as the data size
+ option.with_content_length(bs.len() as u64);
+ op.write_with("path_to_file", option, bs).await?;
+}
+```
+
+The files created by the append operation can be appended via multipart API or append API.
Review Comment:
Do you mean that we should keep the current write API design? Won't this cause confusion for users? We now have two append-like APIs.
--
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 #2133: RFC-2133: Introduce Append API
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #2133:
URL: https://github.com/apache/incubator-opendal/pull/2133#discussion_r1179006397
##########
core/src/docs/rfcs/2133_append_api.md:
##########
@@ -0,0 +1,106 @@
+- Proposal Name: `append_api`
+- Start Date: 2023-04-26
+- RFC PR: [apache/incubator-opendal#2133](https://github.com/apache/incubator-opendal/pull/2133)
+- Tracking Issue:
+
+# Summary
+
+Introduce append operations for OpenDAL which allow users to add data to a file. And write operation will be not reentrant.
+
+# Motivation
+
+OpenDAL has the write operation used to create a file and append data to it. This is implemented based on multipart API. However, current approach has some limitations:
+
+- Data could be lost and not readable before w.close() returned Ok(())
+- File can't be appended again after w.close() returned Ok(())
+
+To address these issues, I propose adding an append operation and modifying the write operation to verify consistency between content length and data size. The revised write operation will not be reentrant; however, users can create an appender that provides a reentrant append operation.
+
+# Guide-level explanation
+
+The files created by the write operation can not be modified, only overwritten.
+
+```rust
+async fn write_test(op: Operation) -> Result<()> {
+ let bs = read_from_file();
+ op.write("path_to_file", bs).await?;
+
+ // The following code will report an error
+ op.write("path_to_file", bs).await?;
+
+ // Unless specified can overwrite
+ let option = OpWrite::default().with_allow_overwrite(true);
+ // content length must be the same as the data size
+ option.with_content_length(bs.len() as u64);
+ op.write_with("path_to_file", option, bs).await?;
+}
+```
+
+The files created by the append operation can be appended via multipart API or append API.
Review Comment:
No, there is no `append` in `write`.
--
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 #2133: RFC-2133: Introduce Append API
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #2133:
URL: https://github.com/apache/incubator-opendal/pull/2133#discussion_r1179006397
##########
core/src/docs/rfcs/2133_append_api.md:
##########
@@ -0,0 +1,106 @@
+- Proposal Name: `append_api`
+- Start Date: 2023-04-26
+- RFC PR: [apache/incubator-opendal#2133](https://github.com/apache/incubator-opendal/pull/2133)
+- Tracking Issue:
+
+# Summary
+
+Introduce append operations for OpenDAL which allow users to add data to a file. And write operation will be not reentrant.
+
+# Motivation
+
+OpenDAL has the write operation used to create a file and append data to it. This is implemented based on multipart API. However, current approach has some limitations:
+
+- Data could be lost and not readable before w.close() returned Ok(())
+- File can't be appended again after w.close() returned Ok(())
+
+To address these issues, I propose adding an append operation and modifying the write operation to verify consistency between content length and data size. The revised write operation will not be reentrant; however, users can create an appender that provides a reentrant append operation.
+
+# Guide-level explanation
+
+The files created by the write operation can not be modified, only overwritten.
+
+```rust
+async fn write_test(op: Operation) -> Result<()> {
+ let bs = read_from_file();
+ op.write("path_to_file", bs).await?;
+
+ // The following code will report an error
+ op.write("path_to_file", bs).await?;
+
+ // Unless specified can overwrite
+ let option = OpWrite::default().with_allow_overwrite(true);
+ // content length must be the same as the data size
+ option.with_content_length(bs.len() as u64);
+ op.write_with("path_to_file", option, bs).await?;
+}
+```
+
+The files created by the append operation can be appended via multipart API or append API.
Review Comment:
No, there is no `append` in `write`. Maybe need to update with `main` branch?
--
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] ShadowySpirits commented on a diff in pull request #2133: RFC-2133: Introduce Append API
Posted by "ShadowySpirits (via GitHub)" <gi...@apache.org>.
ShadowySpirits commented on code in PR #2133:
URL: https://github.com/apache/incubator-opendal/pull/2133#discussion_r1179012636
##########
core/src/docs/rfcs/2133_append_api.md:
##########
@@ -0,0 +1,106 @@
+- Proposal Name: `append_api`
+- Start Date: 2023-04-26
+- RFC PR: [apache/incubator-opendal#2133](https://github.com/apache/incubator-opendal/pull/2133)
+- Tracking Issue:
+
+# Summary
+
+Introduce append operations for OpenDAL which allow users to add data to a file. And write operation will be not reentrant.
+
+# Motivation
+
+OpenDAL has the write operation used to create a file and append data to it. This is implemented based on multipart API. However, current approach has some limitations:
+
+- Data could be lost and not readable before w.close() returned Ok(())
+- File can't be appended again after w.close() returned Ok(())
+
+To address these issues, I propose adding an append operation and modifying the write operation to verify consistency between content length and data size. The revised write operation will not be reentrant; however, users can create an appender that provides a reentrant append operation.
+
+# Guide-level explanation
+
+The files created by the write operation can not be modified, only overwritten.
+
+```rust
+async fn write_test(op: Operation) -> Result<()> {
+ let bs = read_from_file();
+ op.write("path_to_file", bs).await?;
+
+ // The following code will report an error
+ op.write("path_to_file", bs).await?;
+
+ // Unless specified can overwrite
+ let option = OpWrite::default().with_allow_overwrite(true);
+ // content length must be the same as the data size
+ option.with_content_length(bs.len() as u64);
+ op.write_with("path_to_file", option, bs).await?;
+}
+```
+
+The files created by the append operation can be appended via multipart API or append API.
Review Comment:
> No, there is no `append` in `write`.
But the user could write multiple times on the same file. If someone do not understand the difference between multipart and append API, he may be confused.
> Maybe need to update with `main` branch?
I have rebased onto the main branch. The failed ci was caused by a network issue...
<img width="576" alt="image" src="https://user-images.githubusercontent.com/32436597/234847763-08568aeb-08ec-41e8-9470-3bac2b89c907.png">
--
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 #2133: RFC-2133: Introduce Append API
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #2133:
URL: https://github.com/apache/incubator-opendal/pull/2133#discussion_r1177834905
##########
core/src/docs/rfcs/2133_introduce_append_api.md:
##########
@@ -0,0 +1,100 @@
+- Proposal Name: `introduce_append_api`
+- Start Date: 2023-04-26
+- RFC PR: https://github.com/apache/incubator-opendal/pull/2133
+- Tracking Issue:
+
+# Summary
+
+Separate append operations from the write operation.
+
+# Motivation
+
+OpenDAL has the write operation used to create a file and append data to it. This is implemented based on multipart API. However, current approach has some limitations:
+
+- Data could be lost and not readable before w.close() returned Ok(())
+- File can't be appended again after w.close() returned Ok(())
+
+To address these issues, I propose separating the write and append operations. The new write operation will not be reentrant, but users can create a writer that provides a reentrant append operation.
+
+# Guide-level explanation
+
+The files created by the write operation can not be modified, only overwritten.
+
+```rust
+async fn write_test(op: Operation) -> Result<()> {
+ let bs = read_from_file();
+ op.write("path_to_file", bs).await?;
+
+ // The following code will report an error
+ op.write("path_to_file", bs).await?;
+
+ // Unless specified can overwrite
+ let option = OpWrite::default().with_allow_overwrite(true);
+ op.write_with("path_to_file", option, bs).await?;
+}
+```
+
+The files created by the append operation can be appended via multipart API or append API.
+
+```rust
+async fn append_test(op: Operation) -> Result<()> {
+ // create writer
+ let writer = op.writer("path_to_file").await?;
+
+ let bs = read_from_file();
+ writer.append(bs).await?;
+
+ let bs = read_from_another_file();
+ writer.append(bs).await?;
+
+ // close the file
+ // for multipart API, complete the upload and prevent writing after close
+ // for native append API, this is a no-op
+ writer.close().await?;
+
+ // abort the operation and delete all uploaded data
+ writer.abort().await?;
+}
+```
+
+# Reference-level explanation
+
+To implement this feature, we need to add a new API `append` into `oio::Writer`. And change the `oio::Writer::write` to be not reentrant.
+
+```rust
+#[async_trait]
+pub trait Write: Unpin + Send + Sync {
+ /// Write given into writer. This operation will overwrite the file if it is supported.
+ async fn write(&mut self, bs: Bytes) -> Result<()>;
+
+ /// Append data to the end of file.
+ /// Users will call `append` multiple times. Please make sure `append` is safe to re-enter.
+ async fn append(&mut self, bs: Bytes) -> Result<()>;
Review Comment:
> The interface exposed to the user is described in the `Guide-level explanation` section. Please correct me if my understanding is wrong.
Raw API: `oio::Append`
```rust
#[async_trait]
pub trait Append: Unpin + Send + Sync {
async fn append(&mut self, bs: Bytes) -> Result<()>;
async fn close(&mut self) -> Result<()>;
}
```
Public API: `Appender`
```rust
let ap: Appender = op.append("path/to.file").await?;
ap.append(bs).await?;
ap.append(bs).await?;
ap.close().await?;
```
Users can't call `write` on `Appender`, neither call `append` on `Writer`.
--
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] ShadowySpirits commented on a diff in pull request #2133: RFC-2133: Introduce Append API
Posted by "ShadowySpirits (via GitHub)" <gi...@apache.org>.
ShadowySpirits commented on code in PR #2133:
URL: https://github.com/apache/incubator-opendal/pull/2133#discussion_r1177841482
##########
core/src/docs/rfcs/2133_introduce_append_api.md:
##########
@@ -0,0 +1,100 @@
+- Proposal Name: `introduce_append_api`
+- Start Date: 2023-04-26
+- RFC PR: https://github.com/apache/incubator-opendal/pull/2133
+- Tracking Issue:
+
+# Summary
+
+Separate append operations from the write operation.
+
+# Motivation
+
+OpenDAL has the write operation used to create a file and append data to it. This is implemented based on multipart API. However, current approach has some limitations:
+
+- Data could be lost and not readable before w.close() returned Ok(())
+- File can't be appended again after w.close() returned Ok(())
+
+To address these issues, I propose separating the write and append operations. The new write operation will not be reentrant, but users can create a writer that provides a reentrant append operation.
+
+# Guide-level explanation
+
+The files created by the write operation can not be modified, only overwritten.
+
+```rust
+async fn write_test(op: Operation) -> Result<()> {
+ let bs = read_from_file();
+ op.write("path_to_file", bs).await?;
+
+ // The following code will report an error
+ op.write("path_to_file", bs).await?;
+
+ // Unless specified can overwrite
+ let option = OpWrite::default().with_allow_overwrite(true);
+ op.write_with("path_to_file", option, bs).await?;
+}
+```
+
+The files created by the append operation can be appended via multipart API or append API.
+
+```rust
+async fn append_test(op: Operation) -> Result<()> {
+ // create writer
+ let writer = op.writer("path_to_file").await?;
+
+ let bs = read_from_file();
+ writer.append(bs).await?;
+
+ let bs = read_from_another_file();
+ writer.append(bs).await?;
+
+ // close the file
+ // for multipart API, complete the upload and prevent writing after close
+ // for native append API, this is a no-op
+ writer.close().await?;
+
+ // abort the operation and delete all uploaded data
+ writer.abort().await?;
+}
+```
+
+# Reference-level explanation
+
+To implement this feature, we need to add a new API `append` into `oio::Writer`. And change the `oio::Writer::write` to be not reentrant.
+
+```rust
+#[async_trait]
+pub trait Write: Unpin + Send + Sync {
+ /// Write given into writer. This operation will overwrite the file if it is supported.
+ async fn write(&mut self, bs: Bytes) -> Result<()>;
+
+ /// Append data to the end of file.
+ /// Users will call `append` multiple times. Please make sure `append` is safe to re-enter.
+ async fn append(&mut self, bs: Bytes) -> Result<()>;
Review Comment:
Ok, I will follow your suggestion. By the side, what is the `oio` meaning?
--
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 #2133: RFC-2133: Introduce Append API
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #2133:
URL: https://github.com/apache/incubator-opendal/pull/2133#discussion_r1179016649
##########
core/src/docs/rfcs/2133_append_api.md:
##########
@@ -0,0 +1,106 @@
+- Proposal Name: `append_api`
+- Start Date: 2023-04-26
+- RFC PR: [apache/incubator-opendal#2133](https://github.com/apache/incubator-opendal/pull/2133)
+- Tracking Issue:
+
+# Summary
+
+Introduce append operations for OpenDAL which allow users to add data to a file. And write operation will be not reentrant.
+
+# Motivation
+
+OpenDAL has the write operation used to create a file and append data to it. This is implemented based on multipart API. However, current approach has some limitations:
+
+- Data could be lost and not readable before w.close() returned Ok(())
+- File can't be appended again after w.close() returned Ok(())
+
+To address these issues, I propose adding an append operation and modifying the write operation to verify consistency between content length and data size. The revised write operation will not be reentrant; however, users can create an appender that provides a reentrant append operation.
+
+# Guide-level explanation
+
+The files created by the write operation can not be modified, only overwritten.
+
+```rust
+async fn write_test(op: Operation) -> Result<()> {
+ let bs = read_from_file();
+ op.write("path_to_file", bs).await?;
+
+ // The following code will report an error
+ op.write("path_to_file", bs).await?;
+
+ // Unless specified can overwrite
+ let option = OpWrite::default().with_allow_overwrite(true);
+ // content length must be the same as the data size
+ option.with_content_length(bs.len() as u64);
+ op.write_with("path_to_file", option, bs).await?;
+}
+```
+
+The files created by the append operation can be appended via multipart API or append API.
Review Comment:
> But the user could write multiple times on the same file. If someone do not understand
the difference between multipart and append API, he may be confused.
Users won't know the different behavior between underlying services' `multipart` and `append`. They only know about the API provided by OpenDAL: `write` and `append`.
They have different sementics:
- `write`: Always create a new file, not readable unitl `close`.
- `append`: Can append existing file, readable after `append` return.
---
I have to make it clear that: please don't mix opendal's public API and underlying services API. Services can pick up different APIs to implement opendal's API. But our design needs to make it sure that users won't affected by the implement details.
--
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] ShadowySpirits commented on a diff in pull request #2133: RFC-2133: Introduce Append API
Posted by "ShadowySpirits (via GitHub)" <gi...@apache.org>.
ShadowySpirits commented on code in PR #2133:
URL: https://github.com/apache/incubator-opendal/pull/2133#discussion_r1177811557
##########
core/src/docs/rfcs/2133_introduce_append_api.md:
##########
@@ -0,0 +1,100 @@
+- Proposal Name: `introduce_append_api`
+- Start Date: 2023-04-26
+- RFC PR: https://github.com/apache/incubator-opendal/pull/2133
+- Tracking Issue:
+
+# Summary
+
+Separate append operations from the write operation.
+
+# Motivation
+
+OpenDAL has the write operation used to create a file and append data to it. This is implemented based on multipart API. However, current approach has some limitations:
+
+- Data could be lost and not readable before w.close() returned Ok(())
+- File can't be appended again after w.close() returned Ok(())
+
+To address these issues, I propose separating the write and append operations. The new write operation will not be reentrant, but users can create a writer that provides a reentrant append operation.
+
+# Guide-level explanation
+
+The files created by the write operation can not be modified, only overwritten.
+
+```rust
+async fn write_test(op: Operation) -> Result<()> {
+ let bs = read_from_file();
+ op.write("path_to_file", bs).await?;
+
+ // The following code will report an error
+ op.write("path_to_file", bs).await?;
+
+ // Unless specified can overwrite
+ let option = OpWrite::default().with_allow_overwrite(true);
+ op.write_with("path_to_file", option, bs).await?;
+}
+```
+
+The files created by the append operation can be appended via multipart API or append API.
+
+```rust
+async fn append_test(op: Operation) -> Result<()> {
+ // create writer
+ let writer = op.writer("path_to_file").await?;
+
+ let bs = read_from_file();
+ writer.append(bs).await?;
+
+ let bs = read_from_another_file();
+ writer.append(bs).await?;
+
+ // close the file
+ // for multipart API, complete the upload and prevent writing after close
+ // for native append API, this is a no-op
+ writer.close().await?;
+
+ // abort the operation and delete all uploaded data
+ writer.abort().await?;
+}
+```
+
+# Reference-level explanation
+
+To implement this feature, we need to add a new API `append` into `oio::Writer`. And change the `oio::Writer::write` to be not reentrant.
+
+```rust
+#[async_trait]
+pub trait Write: Unpin + Send + Sync {
+ /// Write given into writer. This operation will overwrite the file if it is supported.
+ async fn write(&mut self, bs: Bytes) -> Result<()>;
+
+ /// Append data to the end of file.
+ /// Users will call `append` multiple times. Please make sure `append` is safe to re-enter.
+ async fn append(&mut self, bs: Bytes) -> Result<()>;
Review Comment:
IMO, this trait is an internal API. The interface exposed to the user is described in the Guide-level explanation section. Please correct me if my understanding is wrong.
--
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 #2133: RFC-2133: Introduce Append API
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #2133:
URL: https://github.com/apache/incubator-opendal/pull/2133#discussion_r1177842756
##########
core/src/docs/rfcs/2133_introduce_append_api.md:
##########
@@ -0,0 +1,100 @@
+- Proposal Name: `introduce_append_api`
+- Start Date: 2023-04-26
+- RFC PR: https://github.com/apache/incubator-opendal/pull/2133
+- Tracking Issue:
+
+# Summary
+
+Separate append operations from the write operation.
+
+# Motivation
+
+OpenDAL has the write operation used to create a file and append data to it. This is implemented based on multipart API. However, current approach has some limitations:
+
+- Data could be lost and not readable before w.close() returned Ok(())
+- File can't be appended again after w.close() returned Ok(())
+
+To address these issues, I propose separating the write and append operations. The new write operation will not be reentrant, but users can create a writer that provides a reentrant append operation.
+
+# Guide-level explanation
+
+The files created by the write operation can not be modified, only overwritten.
+
+```rust
+async fn write_test(op: Operation) -> Result<()> {
+ let bs = read_from_file();
+ op.write("path_to_file", bs).await?;
+
+ // The following code will report an error
+ op.write("path_to_file", bs).await?;
+
+ // Unless specified can overwrite
+ let option = OpWrite::default().with_allow_overwrite(true);
+ op.write_with("path_to_file", option, bs).await?;
+}
+```
+
+The files created by the append operation can be appended via multipart API or append API.
+
+```rust
+async fn append_test(op: Operation) -> Result<()> {
+ // create writer
+ let writer = op.writer("path_to_file").await?;
+
+ let bs = read_from_file();
+ writer.append(bs).await?;
+
+ let bs = read_from_another_file();
+ writer.append(bs).await?;
+
+ // close the file
+ // for multipart API, complete the upload and prevent writing after close
+ // for native append API, this is a no-op
+ writer.close().await?;
+
+ // abort the operation and delete all uploaded data
+ writer.abort().await?;
+}
+```
+
+# Reference-level explanation
+
+To implement this feature, we need to add a new API `append` into `oio::Writer`. And change the `oio::Writer::write` to be not reentrant.
+
+```rust
+#[async_trait]
+pub trait Write: Unpin + Send + Sync {
+ /// Write given into writer. This operation will overwrite the file if it is supported.
+ async fn write(&mut self, bs: Bytes) -> Result<()>;
+
+ /// Append data to the end of file.
+ /// Users will call `append` multiple times. Please make sure `append` is safe to re-enter.
+ async fn append(&mut self, bs: Bytes) -> Result<()>;
Review Comment:
> By the side, what is the `oio` meaning?
`oio` is an internal module of opendal: `opendal::raw::oio` which means `OpenDAL IO`
--
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 merged pull request #2133: RFC-2133: Introduce Append API
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo merged PR #2133:
URL: https://github.com/apache/incubator-opendal/pull/2133
--
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 #2133: RFC-2133: Introduce Append API
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #2133:
URL: https://github.com/apache/incubator-opendal/pull/2133#discussion_r1179889522
##########
core/src/docs/rfcs/2133_append_api.md:
##########
@@ -0,0 +1,88 @@
+- Proposal Name: `append_api`
+- Start Date: 2023-04-26
+- RFC PR: [apache/incubator-opendal#2133](https://github.com/apache/incubator-opendal/pull/2133)
+- Tracking Issue:
Review Comment:
Great! Mostly LGTM! Please create a tracking issue and update 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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org