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/09/11 08:49:35 UTC
[GitHub] [incubator-opendal] Xuanwo opened a new pull request, #3029: refactor: Refactor oio::Write into poll-based to create more room for optimization
Xuanwo opened a new pull request, #3029:
URL: https://github.com/apache/incubator-opendal/pull/3029
(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] Xuanwo commented on a diff in pull request #3029: refactor: Refactor oio::Write into poll-based to create more room for optimization
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #3029:
URL: https://github.com/apache/incubator-opendal/pull/3029#discussion_r1321344181
##########
core/src/services/gdrive/writer.rs:
##########
@@ -59,13 +58,7 @@ impl GdriveWriter {
match status {
StatusCode::OK | StatusCode::CREATED => {
- let bs = resp.into_body().bytes().await?;
-
- let file = serde_json::from_slice::<GdriveFile>(&bs)
- .map_err(new_json_deserialize_error)?;
-
- self.file_id = Some(file.id);
-
+ resp.into_body().consume().await?;
Review Comment:
There is no way to reuse the file id 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
[GitHub] [incubator-opendal] suyanhanx commented on a diff in pull request #3029: refactor: Refactor oio::Write into poll-based to create more room for optimization
Posted by "suyanhanx (via GitHub)" <gi...@apache.org>.
suyanhanx commented on code in PR #3029:
URL: https://github.com/apache/incubator-opendal/pull/3029#discussion_r1321404702
##########
core/src/services/gdrive/writer.rs:
##########
@@ -59,13 +58,7 @@ impl GdriveWriter {
match status {
StatusCode::OK | StatusCode::CREATED => {
- let bs = resp.into_body().bytes().await?;
-
- let file = serde_json::from_slice::<GdriveFile>(&bs)
- .map_err(new_json_deserialize_error)?;
-
- self.file_id = Some(file.id);
-
+ resp.into_body().consume().await?;
Review Comment:
Ok. But we can still make up the test.
--
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 #3029: refactor: Refactor oio::Write into poll-based to create more room for optimization
Posted by "suyanhanx (via GitHub)" <gi...@apache.org>.
suyanhanx commented on code in PR #3029:
URL: https://github.com/apache/incubator-opendal/pull/3029#discussion_r1321395845
##########
core/src/services/gdrive/writer.rs:
##########
@@ -59,13 +58,7 @@ impl GdriveWriter {
match status {
StatusCode::OK | StatusCode::CREATED => {
- let bs = resp.into_body().bytes().await?;
-
- let file = serde_json::from_slice::<GdriveFile>(&bs)
- .map_err(new_json_deserialize_error)?;
-
- self.file_id = Some(file.id);
-
+ resp.into_body().consume().await?;
Review Comment:
I checked the test case. This situation wasn't covered.
For a one-shot writer, we do the following steps:
```rust
let w = op.writer("path/a");
w.write("hello");
w.write("world");
w.close();
```
Then the end result should be just one file with the content "world".
But gdrive allows files to hold the same name. So if we don't use the file_id which belongs to the file created by the first write step, the result will be two files.
--
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 #3029: refactor: Refactor oio::Write into poll-based to create more room for optimization
Posted by "suyanhanx (via GitHub)" <gi...@apache.org>.
suyanhanx commented on code in PR #3029:
URL: https://github.com/apache/incubator-opendal/pull/3029#discussion_r1321346481
##########
core/src/services/gdrive/writer.rs:
##########
@@ -59,13 +58,7 @@ impl GdriveWriter {
match status {
StatusCode::OK | StatusCode::CREATED => {
- let bs = resp.into_body().bytes().await?;
-
- let file = serde_json::from_slice::<GdriveFile>(&bs)
- .map_err(new_json_deserialize_error)?;
-
- self.file_id = Some(file.id);
-
+ resp.into_body().consume().await?;
Review Comment:
A writer which is called multiple times needs this.
--
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 #3029: refactor: Refactor oio::Write into poll-based to create more room for optimization
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #3029:
URL: https://github.com/apache/incubator-opendal/pull/3029#discussion_r1321363151
##########
core/src/services/gdrive/writer.rs:
##########
@@ -59,13 +58,7 @@ impl GdriveWriter {
match status {
StatusCode::OK | StatusCode::CREATED => {
- let bs = resp.into_body().bytes().await?;
-
- let file = serde_json::from_slice::<GdriveFile>(&bs)
- .map_err(new_json_deserialize_error)?;
-
- self.file_id = Some(file.id);
-
+ resp.into_body().consume().await?;
Review Comment:
> As gdrive allows files to hold the same name, without reusing the file id, it will be a problem.
I didn't get your idea. Maybe you can add a test case to cover them? I think it's not part of this PR.
--
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 #3029: refactor: Refactor oio::Write into poll-based to create more room for optimization
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #3029:
URL: https://github.com/apache/incubator-opendal/pull/3029#discussion_r1321349905
##########
core/src/services/gdrive/writer.rs:
##########
@@ -59,13 +58,7 @@ impl GdriveWriter {
match status {
StatusCode::OK | StatusCode::CREATED => {
- let bs = resp.into_body().bytes().await?;
-
- let file = serde_json::from_slice::<GdriveFile>(&bs)
- .map_err(new_json_deserialize_error)?;
-
- self.file_id = Some(file.id);
-
+ resp.into_body().consume().await?;
Review Comment:
> A writer which is called multiple times needs this.
GdriveWriter doesn't support muliple write yet which means it will only be called once. Welcome to add it back when we support muliple 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 #3029: refactor: Refactor oio::Write into poll-based to create more room for optimization
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #3029:
URL: https://github.com/apache/incubator-opendal/pull/3029#discussion_r1321344766
##########
core/src/services/gcs/writer.rs:
##########
@@ -74,8 +69,11 @@ impl GcsWriter {
_ => Err(parse_error(resp).await?),
}
}
+}
- async fn initiate_upload(&self) -> Result<String> {
+#[async_trait]
+impl oio::RangeWrite for GcsWriter {
Review Comment:
Already covered by gcs's behavior test. And we can add unit for `RangeWrite` later.
--
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 #3029: refactor: Refactor oio::Write into poll-based to create more room for optimization
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #3029:
URL: https://github.com/apache/incubator-opendal/pull/3029#discussion_r1321405287
##########
core/src/services/gdrive/writer.rs:
##########
@@ -59,13 +58,7 @@ impl GdriveWriter {
match status {
StatusCode::OK | StatusCode::CREATED => {
- let bs = resp.into_body().bytes().await?;
-
- let file = serde_json::from_slice::<GdriveFile>(&bs)
- .map_err(new_json_deserialize_error)?;
-
- self.file_id = Some(file.id);
-
+ resp.into_body().consume().await?;
Review Comment:
> Ok. But we can still make up the test.
Of course, welcome to submit a PR for this.
--
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 #3029: refactor: Refactor oio::Write into poll-based to create more room for optimization
Posted by "suyanhanx (via GitHub)" <gi...@apache.org>.
suyanhanx commented on code in PR #3029:
URL: https://github.com/apache/incubator-opendal/pull/3029#discussion_r1321328946
##########
core/src/services/gdrive/writer.rs:
##########
@@ -59,13 +58,7 @@ impl GdriveWriter {
match status {
StatusCode::OK | StatusCode::CREATED => {
- let bs = resp.into_body().bytes().await?;
-
- let file = serde_json::from_slice::<GdriveFile>(&bs)
- .map_err(new_json_deserialize_error)?;
-
- self.file_id = Some(file.id);
-
+ resp.into_body().consume().await?;
Review Comment:
？We need this file_id
##########
core/src/raw/oio/write/range_write.rs:
##########
@@ -0,0 +1,292 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use std::task::ready;
+use std::task::Context;
+use std::task::Poll;
+
+use async_trait::async_trait;
+use futures::future::BoxFuture;
+use futures::FutureExt;
+
+use crate::raw::oio::WriteBuf;
+use crate::raw::*;
+use crate::*;
+
+/// RangeWrite is used to implement [`Write`] based on range write.
+///
+/// # Services
+///
+/// Services like gcs support range write via [GCS Resumable Upload](https://cloud.google.com/storage/docs/resumable-uploads).
+///
+/// GCS will support upload content by specifying the range of the file in `CONTENT-RANGE`.
+///
+/// Most range based services will have the following limitations:
+///
+/// - The size of chunk per upload must be aligned to a certain size. For example, GCS requires
+/// to align with 256KiB.
+/// - Some services requires to complete the write at the last chunk with the total size.
+///
+/// # Architecture
+///
+/// The architecture after adopting [`RangeWrite`]:
+///
+/// - Services impl `RangeWrite`
+/// - `RangeWriter` impl `Write`
+/// - Expose `RangeWriter` as `Accessor::Writer`
+#[async_trait]
+pub trait RangeWrite: Send + Sync + Unpin + 'static {
Review Comment:
This name is a bit confusing. I thought we could overwrite just part of the file, but we could not in fact.
##########
core/src/services/gdrive/writer.rs:
##########
@@ -59,13 +58,7 @@ impl GdriveWriter {
match status {
StatusCode::OK | StatusCode::CREATED => {
- let bs = resp.into_body().bytes().await?;
-
- let file = serde_json::from_slice::<GdriveFile>(&bs)
- .map_err(new_json_deserialize_error)?;
-
- self.file_id = Some(file.id);
-
+ resp.into_body().consume().await?;
Review Comment:
？We need this file_id
##########
core/src/services/gcs/writer.rs:
##########
@@ -74,8 +69,11 @@ impl GcsWriter {
_ => Err(parse_error(resp).await?),
}
}
+}
- async fn initiate_upload(&self) -> Result<String> {
+#[async_trait]
+impl oio::RangeWrite for GcsWriter {
Review Comment:
We need a test case for it.
--
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 #3029: refactor: Refactor oio::Write into poll-based to create more room for optimization
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #3029:
URL: https://github.com/apache/incubator-opendal/pull/3029#discussion_r1321348591
##########
core/src/raw/oio/write/range_write.rs:
##########
@@ -0,0 +1,292 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use std::task::ready;
+use std::task::Context;
+use std::task::Poll;
+
+use async_trait::async_trait;
+use futures::future::BoxFuture;
+use futures::FutureExt;
+
+use crate::raw::oio::WriteBuf;
+use crate::raw::*;
+use crate::*;
+
+/// RangeWrite is used to implement [`Write`] based on range write.
+///
+/// # Services
+///
+/// Services like gcs support range write via [GCS Resumable Upload](https://cloud.google.com/storage/docs/resumable-uploads).
+///
+/// GCS will support upload content by specifying the range of the file in `CONTENT-RANGE`.
+///
+/// Most range based services will have the following limitations:
+///
+/// - The size of chunk per upload must be aligned to a certain size. For example, GCS requires
+/// to align with 256KiB.
+/// - Some services requires to complete the write at the last chunk with the total size.
+///
+/// # Architecture
+///
+/// The architecture after adopting [`RangeWrite`]:
+///
+/// - Services impl `RangeWrite`
+/// - `RangeWriter` impl `Write`
+/// - Expose `RangeWriter` as `Accessor::Writer`
+#[async_trait]
+pub trait RangeWrite: Send + Sync + Unpin + 'static {
Review Comment:
Do you have better names? Anyway, it's an internal trait, welcome to submit a PR to polish it.
--
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 #3029: refactor: Refactor oio::Write into poll-based to create more room for optimization
Posted by "suyanhanx (via GitHub)" <gi...@apache.org>.
suyanhanx commented on code in PR #3029:
URL: https://github.com/apache/incubator-opendal/pull/3029#discussion_r1321395845
##########
core/src/services/gdrive/writer.rs:
##########
@@ -59,13 +58,7 @@ impl GdriveWriter {
match status {
StatusCode::OK | StatusCode::CREATED => {
- let bs = resp.into_body().bytes().await?;
-
- let file = serde_json::from_slice::<GdriveFile>(&bs)
- .map_err(new_json_deserialize_error)?;
-
- self.file_id = Some(file.id);
-
+ resp.into_body().consume().await?;
Review Comment:
I checked the test case. This situation wasn't covered.
For a one-shot writer, we do the following steps:
```rust
let w = op.writer("path/a");
w.write("hello");
w.write("world");
w.close();
```
Then the end result should be just one file with the content "world".
But gdrive allows files to hold the same name/path. So if we don't use the file_id which belongs to the file created by the first write step, the result will be two files.
--
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] silver-ymz commented on a diff in pull request #3029: refactor: Refactor oio::Write into poll-based to create more room for optimization
Posted by "silver-ymz (via GitHub)" <gi...@apache.org>.
silver-ymz commented on code in PR #3029:
URL: https://github.com/apache/incubator-opendal/pull/3029#discussion_r1321464442
##########
core/src/services/sftp/writer.rs:
##########
@@ -15,38 +15,51 @@
// specific language governing permissions and limitations
// under the License.
+use std::pin::Pin;
+use std::task::Context;
+use std::task::Poll;
+
use async_trait::async_trait;
use openssh_sftp_client::file::File;
+use openssh_sftp_client::file::TokioCompatFile;
+use tokio::io::AsyncWrite;
use crate::raw::oio;
use crate::*;
pub struct SftpWriter {
- file: File,
+ file: Pin<Box<TokioCompatFile>>,
}
impl SftpWriter {
pub fn new(file: File) -> Self {
- SftpWriter { file }
+ SftpWriter {
+ file: Box::pin(TokioCompatFile::new(file)),
+ }
}
}
#[async_trait]
impl oio::Write for SftpWriter {
- async fn write(&mut self, bs: &dyn oio::WriteBuf) -> Result<usize> {
- let size = self.file.write(bs.chunk()).await?;
+ fn poll_write(&mut self, cx: &mut Context<'_>, bs: &dyn oio::WriteBuf) -> Poll<Result<usize>> {
+ self.file
+ .as_mut()
+ .poll_write(cx, bs.chunk())
+ .map_err(parse_io_error)
+ }
- Ok(size)
+ fn poll_close(&mut self, cx: &mut Context<'_>) -> Poll<Result<()>> {
+ self.file.as_mut().poll_shutdown(cx).map_err(parse_io_error)
}
- async fn abort(&mut self) -> Result<()> {
- Err(Error::new(
+ fn poll_abort(&mut self, _: &mut Context<'_>) -> Poll<Result<()>> {
+ Poll::Ready(Err(Error::new(
ErrorKind::Unsupported,
- "SFTP does not support aborting writes",
- ))
+ "HdfsWriter doesn't support abort",
Review Comment:
This should be "SftpWriter doesn't support abort"
--
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 #3029: refactor: Refactor oio::Write into poll-based to create more room for optimization
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo merged PR #3029:
URL: https://github.com/apache/incubator-opendal/pull/3029
--
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 #3029: refactor: Refactor oio::Write into poll-based to create more room for optimization
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #3029:
URL: https://github.com/apache/incubator-opendal/pull/3029#discussion_r1321400209
##########
core/src/services/gdrive/writer.rs:
##########
@@ -59,13 +58,7 @@ impl GdriveWriter {
match status {
StatusCode::OK | StatusCode::CREATED => {
- let bs = resp.into_body().bytes().await?;
-
- let file = serde_json::from_slice::<GdriveFile>(&bs)
- .map_err(new_json_deserialize_error)?;
-
- self.file_id = Some(file.id);
-
+ resp.into_body().consume().await?;
Review Comment:
> So if we don't use the file_id which belongs to the file created by the first write step, the result will be two files.
First of all, writer without content length is not supported by gdrive, so which means creating a `writer` in this way will raise an Unsupported error.
Second, our `CompleteLayer` has checked the total content length. Trying to write the same `OneShotWriter` file twice will raise an `ContentExceeded` error.
So, in our current limitation, gdrive doesn' need a file_id to make it behaves correctly.
--
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 #3029: refactor: Refactor oio::Write into poll-based to create more room for optimization
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #3029:
URL: https://github.com/apache/incubator-opendal/pull/3029#discussion_r1321400209
##########
core/src/services/gdrive/writer.rs:
##########
@@ -59,13 +58,7 @@ impl GdriveWriter {
match status {
StatusCode::OK | StatusCode::CREATED => {
- let bs = resp.into_body().bytes().await?;
-
- let file = serde_json::from_slice::<GdriveFile>(&bs)
- .map_err(new_json_deserialize_error)?;
-
- self.file_id = Some(file.id);
-
+ resp.into_body().consume().await?;
Review Comment:
> So if we don't use the file_id which belongs to the file created by the first write step, the result will be two files.
First of all, writer without content length is not supported by gdrive, so which means creating a `writer` in this way will raise an Unsupported error.
Second, our `CompleteLayer` has checked the total content length. Trying to write the same file twice will raise an `ContentExceeded` error.
So, in our current limitation, gdrive doesn' need a file_id to make it behaves correctly.
--
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 #3029: refactor: Refactor oio::Write into poll-based to create more room for optimization
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #3029:
URL: https://github.com/apache/incubator-opendal/pull/3029#discussion_r1321363151
##########
core/src/services/gdrive/writer.rs:
##########
@@ -59,13 +58,7 @@ impl GdriveWriter {
match status {
StatusCode::OK | StatusCode::CREATED => {
- let bs = resp.into_body().bytes().await?;
-
- let file = serde_json::from_slice::<GdriveFile>(&bs)
- .map_err(new_json_deserialize_error)?;
-
- self.file_id = Some(file.id);
-
+ resp.into_body().consume().await?;
Review Comment:
> As gdrive allows files to hold the same name, without reusing the file id, it will be a problem.
I didn't get your idea. Maybe you can add a test case to cover them?
--
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 #3029: refactor: Refactor oio::Write into poll-based to create more room for optimization
Posted by "suyanhanx (via GitHub)" <gi...@apache.org>.
suyanhanx commented on code in PR #3029:
URL: https://github.com/apache/incubator-opendal/pull/3029#discussion_r1321358406
##########
core/src/services/gdrive/writer.rs:
##########
@@ -59,13 +58,7 @@ impl GdriveWriter {
match status {
StatusCode::OK | StatusCode::CREATED => {
- let bs = resp.into_body().bytes().await?;
-
- let file = serde_json::from_slice::<GdriveFile>(&bs)
- .map_err(new_json_deserialize_error)?;
-
- self.file_id = Some(file.id);
-
+ resp.into_body().consume().await?;
Review Comment:
No.Just a normal writer. As gdrive allows files to hold the same name, without reusing the file id, it will be a problem.
--
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 #3029: refactor: Refactor oio::Write into poll-based to create more room for optimization
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #3029:
URL: https://github.com/apache/incubator-opendal/pull/3029#discussion_r1321480338
##########
core/src/services/sftp/writer.rs:
##########
@@ -15,38 +15,51 @@
// specific language governing permissions and limitations
// under the License.
+use std::pin::Pin;
+use std::task::Context;
+use std::task::Poll;
+
use async_trait::async_trait;
use openssh_sftp_client::file::File;
+use openssh_sftp_client::file::TokioCompatFile;
+use tokio::io::AsyncWrite;
use crate::raw::oio;
use crate::*;
pub struct SftpWriter {
- file: File,
+ file: Pin<Box<TokioCompatFile>>,
}
impl SftpWriter {
pub fn new(file: File) -> Self {
- SftpWriter { file }
+ SftpWriter {
+ file: Box::pin(TokioCompatFile::new(file)),
+ }
}
}
#[async_trait]
impl oio::Write for SftpWriter {
- async fn write(&mut self, bs: &dyn oio::WriteBuf) -> Result<usize> {
- let size = self.file.write(bs.chunk()).await?;
+ fn poll_write(&mut self, cx: &mut Context<'_>, bs: &dyn oio::WriteBuf) -> Poll<Result<usize>> {
+ self.file
+ .as_mut()
+ .poll_write(cx, bs.chunk())
+ .map_err(parse_io_error)
+ }
- Ok(size)
+ fn poll_close(&mut self, cx: &mut Context<'_>) -> Poll<Result<()>> {
+ self.file.as_mut().poll_shutdown(cx).map_err(parse_io_error)
}
- async fn abort(&mut self) -> Result<()> {
- Err(Error::new(
+ fn poll_abort(&mut self, _: &mut Context<'_>) -> Poll<Result<()>> {
+ Poll::Ready(Err(Error::new(
ErrorKind::Unsupported,
- "SFTP does not support aborting writes",
- ))
+ "HdfsWriter doesn't support abort",
Review Comment:
> This should be "SftpWriter doesn't support abort"
Oh, my bad. Would you like to help fix this typo?
--
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