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/04/19 09:25:37 UTC
[GitHub] [incubator-opendal] Xuanwo opened a new pull request, #2036: refactor: Polish Writer API by merging append and write together
Xuanwo opened a new pull request, #2036:
URL: https://github.com/apache/incubator-opendal/pull/2036
This PR introduces the following changes:
- Remove `append` from `OpWrite`: this arg is confusing.
- Add `content_length` in `OpWrite` so that services can handle sized and unsized input by themselves
--
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 #2036: refactor: Polish Writer API by merging append and write together
Posted by "suyanhanx (via GitHub)" <gi...@apache.org>.
suyanhanx commented on code in PR #2036:
URL: https://github.com/apache/incubator-opendal/pull/2036#discussion_r1171080522
##########
core/src/types/ops.rs:
##########
@@ -377,17 +382,6 @@ impl OpWrite {
self.cache_control = Some(cache_control.to_string());
self
}
-
- /// Set the If-Match of the option
- pub fn with_if_match(mut self, if_match: &str) -> Self {
- self.if_match = Some(if_match.to_string());
- self
- }
-
- /// Get If-Match from option
- pub fn if_match(&self) -> Option<&str> {
- self.if_match.as_deref()
- }
Review Comment:
Is it right?
##########
core/src/types/ops.rs:
##########
@@ -320,12 +320,10 @@ impl OpStat {
/// Args for `write` operation.
#[derive(Debug, Clone, Default)]
pub struct OpWrite {
- append: bool,
-
+ content_length: Option<u64>,
content_type: Option<String>,
content_disposition: Option<String>,
cache_control: Option<String>,
- if_match: Option<String>,
Review Comment:
why?
--
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 #2036: refactor: Polish Writer API by merging append and write together
Posted by "suyanhanx (via GitHub)" <gi...@apache.org>.
suyanhanx commented on code in PR #2036:
URL: https://github.com/apache/incubator-opendal/pull/2036#discussion_r1171115485
##########
core/src/types/ops.rs:
##########
@@ -320,12 +320,10 @@ impl OpStat {
/// Args for `write` operation.
#[derive(Debug, Clone, Default)]
pub struct OpWrite {
- append: bool,
Review Comment:
If `write` only does like `append`, isn't that a bit different from op's `write` and confusing?
--
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 pull request #2036: refactor: Polish Writer API by merging append and write together
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on PR #2036:
URL: https://github.com/apache/incubator-opendal/pull/2036#issuecomment-1514470181
No, we are confused many things together here.
After this PRs change, there is no `append` concept, we have two different write uses cases: Sized and Unsized.
If users specified the size for `write` operation and provide all data with it, we can write with oneshot. Otherwise, services need to handle it in different ways: append, multipart upload, range write, ...
`op.write()` is exactly this case: we know the size and we have all data.
`op.writer()` is the other case: we don't know the size in advance, and services need to handle it by different ways.
--
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 #2036: refactor: Polish Writer API by merging append and write together
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #2036:
URL: https://github.com/apache/incubator-opendal/pull/2036#discussion_r1171098013
##########
core/src/raw/adapters/kv/backend.rs:
##########
@@ -264,35 +278,17 @@ impl<S> KvWriter<S> {
buf: None,
}
}
-
- fn extend_buf(&mut self, bs: Bytes) {
- if let Some(buf) = self.buf.as_mut() {
- buf.extend(bs);
- } else {
- self.buf = Some(bs.into())
- }
- }
}
#[async_trait]
impl<S: Adapter> oio::Write for KvWriter<S> {
+ // TODO: we need to support append in the future.
Review Comment:
Yep!
--
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 pull request #2036: refactor: Polish Writer API by merging append and write together
Posted by "suyanhanx (via GitHub)" <gi...@apache.org>.
suyanhanx commented on PR #2036:
URL: https://github.com/apache/incubator-opendal/pull/2036#issuecomment-1514459855
If users want to overwrite multiple times and then rollback, they will no longer be able to use the `writer` to do so.
WDYT?
--
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 #2036: refactor: Polish Writer API by merging append and write together
Posted by "suyanhanx (via GitHub)" <gi...@apache.org>.
suyanhanx commented on code in PR #2036:
URL: https://github.com/apache/incubator-opendal/pull/2036#discussion_r1171092357
##########
core/src/raw/adapters/kv/backend.rs:
##########
@@ -264,35 +278,17 @@ impl<S> KvWriter<S> {
buf: None,
}
}
-
- fn extend_buf(&mut self, bs: Bytes) {
- if let Some(buf) = self.buf.as_mut() {
- buf.extend(bs);
- } else {
- self.buf = Some(bs.into())
- }
- }
}
#[async_trait]
impl<S: Adapter> oio::Write for KvWriter<S> {
+ // TODO: we need to support append in the future.
Review Comment:
This needs to happen soon. 😄
--
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 #2036: refactor: Polish Writer API by merging append and write together
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #2036:
URL: https://github.com/apache/incubator-opendal/pull/2036#discussion_r1171086155
##########
core/src/types/ops.rs:
##########
@@ -320,12 +320,10 @@ impl OpStat {
/// Args for `write` operation.
#[derive(Debug, Clone, Default)]
pub struct OpWrite {
- append: bool,
-
+ content_length: Option<u64>,
content_type: Option<String>,
content_disposition: Option<String>,
cache_control: Option<String>,
- if_match: Option<String>,
Review Comment:
`PutObject` doesn't have `If-Match` support.
--
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 #2036: refactor: Polish Writer API by merging append and write together
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo merged PR #2036:
URL: https://github.com/apache/incubator-opendal/pull/2036
--
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 pull request #2036: refactor: Polish Writer API by merging append and write together
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on PR #2036:
URL: https://github.com/apache/incubator-opendal/pull/2036#issuecomment-1514432315
> Seems like you're using `write` instead of `append`. Please append this change to desc.
DONE.
--
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 pull request #2036: refactor: Polish Writer API by merging append and write together
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on PR #2036:
URL: https://github.com/apache/incubator-opendal/pull/2036#issuecomment-1514463169
> If users want to overwrite multiple times and then rollback, they will no longer be able to use the `writer` to do so.
I didn't understand these use cases. Could you provide more 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] Xuanwo commented on a diff in pull request #2036: refactor: Polish Writer API by merging append and write together
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #2036:
URL: https://github.com/apache/incubator-opendal/pull/2036#discussion_r1171087195
##########
core/src/types/ops.rs:
##########
@@ -377,17 +382,6 @@ impl OpWrite {
self.cache_control = Some(cache_control.to_string());
self
}
-
- /// Set the If-Match of the option
- pub fn with_if_match(mut self, if_match: &str) -> Self {
- self.if_match = Some(if_match.to_string());
- self
- }
-
- /// Get If-Match from option
- pub fn if_match(&self) -> Option<&str> {
- self.if_match.as_deref()
- }
Review Comment:
Yes, OpWrite doesn't have `if-match` support.
--
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 pull request #2036: refactor: Polish Writer API by merging append and write together
Posted by "suyanhanx (via GitHub)" <gi...@apache.org>.
suyanhanx commented on PR #2036:
URL: https://github.com/apache/incubator-opendal/pull/2036#issuecomment-1514428930
Seems like you're using `write` instead of `append`. Please append this change to desc.
--
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 pull request #2036: refactor: Polish Writer API by merging append and write together
Posted by "suyanhanx (via GitHub)" <gi...@apache.org>.
suyanhanx commented on PR #2036:
URL: https://github.com/apache/incubator-opendal/pull/2036#issuecomment-1514474440
> No, we are confused many things together here.
>
> After this PRs change, there is no `append` concept, we have two different write uses cases: Sized and Unsized.
>
> If users specified the size for `write` operation and provide all data with it, we can write with oneshot. Otherwise, services need to handle it in different ways: append, multipart upload, range write, ...
>
> `op.write()` is exactly this case: we know the size and we have all data. `op.writer()` is the other case: we don't know the size in advance, and services need to handle it by different ways.
>
> In general, we will hide all the impelment details inside services. The only thing for users to use is `write`, and the only choice for users is specify the size or not.
Great explanation! I think this needs to be in the documentation.
--
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 pull request #2036: refactor: Polish Writer API by merging append and write together
Posted by "suyanhanx (via GitHub)" <gi...@apache.org>.
suyanhanx commented on PR #2036:
URL: https://github.com/apache/incubator-opendal/pull/2036#issuecomment-1514466214
> I didn't understand these use cases. Could you provide more details?
I think this is a problem of mixing `write` and `append` and not being able to distinguish between the two operations.
--
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