You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@opendal.apache.org by "silver-ymz (via GitHub)" <gi...@apache.org> on 2023/05/15 03:50:38 UTC
[GitHub] [incubator-opendal] silver-ymz opened a new pull request, #2263: feat(services/sftp): support copy and rename for sftp
silver-ymz opened a new pull request, #2263:
URL: https://github.com/apache/incubator-opendal/pull/2263
(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 pull request #2263: feat(services/sftp): support copy and rename for sftp
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on PR #2263:
URL: https://github.com/apache/incubator-opendal/pull/2263#issuecomment-1549962642
> But we can't still detect it in `build` function.
May I ask why we need to detect this? Can we return `NotSuppported` error while copy failed for server doesn't support 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] silver-ymz commented on a diff in pull request #2263: feat(services/sftp): support copy and rename for sftp
Posted by "silver-ymz (via GitHub)" <gi...@apache.org>.
silver-ymz commented on code in PR #2263:
URL: https://github.com/apache/incubator-opendal/pull/2263#discussion_r1193342462
##########
core/src/services/sftp/backend.rs:
##########
@@ -375,6 +375,49 @@ impl Accessor for SftpBackend {
Ok((RpWrite::new(), SftpWriter::new(file)))
}
+ async fn copy(&self, from: &str, to: &str, _args: OpCopy) -> Result<RpCopy> {
+ let client = self.connect().await?;
+
+ let mut fs = client.fs();
+ fs.set_cwd(&self.root);
+
+ if let Some((dir, _)) = to.rsplit_once('/') {
+ self.create_dir(dir, OpCreateDir::default()).await?;
+ }
+
+ let src = fs.canonicalize(from).await?;
+ let dst = fs.canonicalize(to).await?;
+ let mut src_file = client.open(&src).await?;
+ let mut dst_file = client.create(dst).await?;
+
+ // if remote sftp server supports copy extension, use it
+ if let Ok(()) = src_file.copy_all_to(&mut dst_file).await {
+ return Ok(RpCopy::default());
+ }
+
+ src_file.close().await?;
+ let buffer = fs.read(&src).await?;
+ dst_file.write_all(&buffer).await?;
+
+ Ok(RpCopy::default())
Review Comment:
Ok. I'll change it. And in `rename` test, only `test_rename_nested` will fail. Is there a way to bypass the test? Or we need to provide `rename` function, but set `rename` capability to false.
--
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 #2263: feat(services/sftp): support copy and rename for sftp
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo merged PR #2263:
URL: https://github.com/apache/incubator-opendal/pull/2263
--
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 pull request #2263: feat(services/sftp): support copy and rename for sftp
Posted by "silver-ymz (via GitHub)" <gi...@apache.org>.
silver-ymz commented on PR #2263:
URL: https://github.com/apache/incubator-opendal/pull/2263#issuecomment-1548094378
> Is the reason you removed the content-length check?
I think it's because I set `read_with_range` to true, and this feature has some 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] suyanhanx commented on pull request #2263: feat(services/sftp): support copy and rename for sftp
Posted by "suyanhanx (via GitHub)" <gi...@apache.org>.
suyanhanx commented on PR #2263:
URL: https://github.com/apache/incubator-opendal/pull/2263#issuecomment-1547239361
Seems that it is possible to rename to a different path if it is a POSIX_RENAME operation.
Is there any way to detect its capability? π€
--
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 #2263: feat(services/sftp): support copy and rename for sftp
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #2263:
URL: https://github.com/apache/incubator-opendal/pull/2263#discussion_r1195153617
##########
core/src/services/sftp/backend.rs:
##########
@@ -55,8 +55,8 @@ use crate::*;
/// - [x] write
/// - [x] create_dir
/// - [x] delete
-/// - [ ] copy
-/// - [ ] rename
+/// - [ ] copy (partially, only supoort when remote server has copy-file extension)
+/// - [x] rename (partially, only support to rename files on the same mount point)
Review Comment:
The same.
##########
core/src/services/sftp/backend.rs:
##########
@@ -375,6 +373,40 @@ impl Accessor for SftpBackend {
Ok((RpWrite::new(), SftpWriter::new(file)))
}
+ async fn copy(&self, from: &str, to: &str, _args: OpCopy) -> Result<RpCopy> {
Review Comment:
Please use `_` instead of `_args` if we don't want to use it.
##########
core/src/services/sftp/backend.rs:
##########
@@ -55,8 +55,8 @@ use crate::*;
/// - [x] write
/// - [x] create_dir
/// - [x] delete
-/// - [ ] copy
-/// - [ ] rename
+/// - [ ] copy (partially, only supoort when remote server has copy-file extension)
Review Comment:
We don't need explain this: OpenDAL already implement all features, it's server's own duty.
--
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 #2263: feat(services/sftp): support copy and rename for sftp
Posted by "suyanhanx (via GitHub)" <gi...@apache.org>.
suyanhanx commented on PR #2263:
URL: https://github.com/apache/incubator-opendal/pull/2263#issuecomment-1547254559
> Should we send a test request when building `sftp` backend to detect the capability? This needs to block an async operation in `build` function.
I think it isn't a big problem. cc @Xuanwo
--
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] NobodyXu commented on pull request #2263: feat(services/sftp): support copy and rename for sftp
Posted by "NobodyXu (via GitHub)" <gi...@apache.org>.
NobodyXu commented on PR #2263:
URL: https://github.com/apache/incubator-opendal/pull/2263#issuecomment-1549884526
> Detecting the capability in build function costs expensively. We need to spawn a new process to initiate a complete connection, and find a file which has read-write permission to test copying. Moreover, all of these things will use block_on function, which makes the interaction with tokio becomes more complex.
@silver-ymz Auxiliary contains [all supported extensios](https://github.com/openssh-rust/openssh-sftp-client/blob/ec5789501971ed6a5bd8382c2d02d185a21f9bb0/src/auxiliary.rs#L18), so you could expose a new function in openssh-sftp-client easily.
--
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 pull request #2263: feat(services/sftp): support copy and rename for sftp
Posted by "silver-ymz (via GitHub)" <gi...@apache.org>.
silver-ymz commented on PR #2263:
URL: https://github.com/apache/incubator-opendal/pull/2263#issuecomment-1547674763
After some research, I think we may need to give up the sftp copy feature. There are 3 reasons:
1. Sftp `copy-file` extension is supported by few SFTP servers only. For example by [ProFTPD mod_sftp](http://www.proftpd.org/docs/contrib/mod_sftp.html) and Bitvise WinSSHD. The most widespread SFTP server, the OpenSSH supports related `copy-data` only in very recent [version 9.0](https://www.openssh.com/txt/release-9.0).
2. There are many ways to copy files except for `sftp`, such as direct `cp` in `ssh`, `scp`. There are very few users who use `sftp` to copy files.
3. Detecting the capability in `build` function costs expensively. We need to spawn a new process to initiate a complete connection, and find a file which has read-write permission to test copying. Moreover, all of these things will use `block_on` function, which makes the interaction with `tokio` becomes more complex.
--
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 #2263: feat(services/sftp): support copy and rename for sftp
Posted by "suyanhanx (via GitHub)" <gi...@apache.org>.
suyanhanx commented on PR #2263:
URL: https://github.com/apache/incubator-opendal/pull/2263#issuecomment-1549039358
> I find the reason. The buffer's length in `TokioCompatFile` of `openssh-sftp-client` is 4096 as default. If we seek the position far than `Current(4096)`, it will only seek to `Current(4096)`. Should we change the impl of `AsyncSeek` in `SftpReaderInner`?
We can open a new issue. You may revert some relevate code and we could do this content-length job in another 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] silver-ymz commented on a diff in pull request #2263: feat(services/sftp): support copy and rename for sftp
Posted by "silver-ymz (via GitHub)" <gi...@apache.org>.
silver-ymz commented on code in PR #2263:
URL: https://github.com/apache/incubator-opendal/pull/2263#discussion_r1193342462
##########
core/src/services/sftp/backend.rs:
##########
@@ -375,6 +375,49 @@ impl Accessor for SftpBackend {
Ok((RpWrite::new(), SftpWriter::new(file)))
}
+ async fn copy(&self, from: &str, to: &str, _args: OpCopy) -> Result<RpCopy> {
+ let client = self.connect().await?;
+
+ let mut fs = client.fs();
+ fs.set_cwd(&self.root);
+
+ if let Some((dir, _)) = to.rsplit_once('/') {
+ self.create_dir(dir, OpCreateDir::default()).await?;
+ }
+
+ let src = fs.canonicalize(from).await?;
+ let dst = fs.canonicalize(to).await?;
+ let mut src_file = client.open(&src).await?;
+ let mut dst_file = client.create(dst).await?;
+
+ // if remote sftp server supports copy extension, use it
+ if let Ok(()) = src_file.copy_all_to(&mut dst_file).await {
+ return Ok(RpCopy::default());
+ }
+
+ src_file.close().await?;
+ let buffer = fs.read(&src).await?;
+ dst_file.write_all(&buffer).await?;
+
+ Ok(RpCopy::default())
Review Comment:
Ok. I'll change it. And because sftp posix rename only supports on files in the same parent dir, in `rename` test, only `test_rename_nested` will fail. Is there a way to bypass the test? Or we need to provide `rename` function, but set `rename` capability to false.
--
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 pull request #2263: feat(services/sftp): support copy and rename for sftp
Posted by "silver-ymz (via GitHub)" <gi...@apache.org>.
silver-ymz commented on PR #2263:
URL: https://github.com/apache/incubator-opendal/pull/2263#issuecomment-1547295377
There is a new bug about `range_read`.
In https://github.com/apache/incubator-opendal/actions/runs/4976970726/jobs/8905529556, test `write_test_fuzz_range_reader` failed.
```
thread 'services_sftp::write_test_fuzz_range_reader' panicked at 'assertion failed: `(left == right)`
left: `546602`,
right: `543843`: check seek failed: output pos is different with expected pos, actions: [Read(84819), Seek(End(-519436)), Seek(Start(346471)), Next, Read(27351), Seek(Start(349302)), Next, Seek(End(-339634)), Next, Read(180661), Seek(End(-561960)), Next, Next, Seek(End(-768061)), Next, Next, Seek(End(-634305)), Next, Next, Read(1337), Seek(End(-328798))]',
```
I'm debuging it now.
--
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 #2263: feat(services/sftp): support copy and rename for sftp
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on PR #2263:
URL: https://github.com/apache/incubator-opendal/pull/2263#issuecomment-1550844434
> So should we add copy to `Capability` in `info` function?
Hi, @silver-ymz, would like to add this in next PR? This PR is good enough to get merged.
--
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 pull request #2263: feat(services/sftp): support copy and rename for sftp
Posted by "silver-ymz (via GitHub)" <gi...@apache.org>.
silver-ymz commented on PR #2263:
URL: https://github.com/apache/incubator-opendal/pull/2263#issuecomment-1547252287
Sorry for misunderstanding the document. We can rename to a different path as long as ensure the destionation dir exists. The unsupported operation is rename to the dir of different mount point. We can pass the `rename` tests now.
But `copy` operation only supports when the remote server has copy-data extension. Should we send a test request when building sftp backend to detect the capability? This needs to block an async operation in `build` function.
--
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 #2263: feat(services/sftp): support copy and rename for sftp
Posted by "suyanhanx (via GitHub)" <gi...@apache.org>.
suyanhanx commented on code in PR #2263:
URL: https://github.com/apache/incubator-opendal/pull/2263#discussion_r1193331688
##########
core/src/services/sftp/backend.rs:
##########
@@ -375,6 +375,49 @@ impl Accessor for SftpBackend {
Ok((RpWrite::new(), SftpWriter::new(file)))
}
+ async fn copy(&self, from: &str, to: &str, _args: OpCopy) -> Result<RpCopy> {
+ let client = self.connect().await?;
+
+ let mut fs = client.fs();
+ fs.set_cwd(&self.root);
+
+ if let Some((dir, _)) = to.rsplit_once('/') {
+ self.create_dir(dir, OpCreateDir::default()).await?;
+ }
+
+ let src = fs.canonicalize(from).await?;
+ let dst = fs.canonicalize(to).await?;
+ let mut src_file = client.open(&src).await?;
+ let mut dst_file = client.create(dst).await?;
+
+ // if remote sftp server supports copy extension, use it
+ if let Ok(()) = src_file.copy_all_to(&mut dst_file).await {
+ return Ok(RpCopy::default());
+ }
+
+ src_file.close().await?;
+ let buffer = fs.read(&src).await?;
+ dst_file.write_all(&buffer).await?;
+
+ Ok(RpCopy::default())
Review Comment:
We prefer to implement it only when the backend is natively supported.
You could return an error so users can handle it themselves.
##########
core/src/services/sftp/backend.rs:
##########
@@ -375,6 +375,49 @@ impl Accessor for SftpBackend {
Ok((RpWrite::new(), SftpWriter::new(file)))
}
+ async fn copy(&self, from: &str, to: &str, _args: OpCopy) -> Result<RpCopy> {
+ let client = self.connect().await?;
+
+ let mut fs = client.fs();
+ fs.set_cwd(&self.root);
+
+ if let Some((dir, _)) = to.rsplit_once('/') {
+ self.create_dir(dir, OpCreateDir::default()).await?;
+ }
+
+ let src = fs.canonicalize(from).await?;
+ let dst = fs.canonicalize(to).await?;
+ let mut src_file = client.open(&src).await?;
+ let mut dst_file = client.create(dst).await?;
+
+ // if remote sftp server supports copy extension, use it
+ if let Ok(()) = src_file.copy_all_to(&mut dst_file).await {
+ return Ok(RpCopy::default());
+ }
+
+ src_file.close().await?;
+ let buffer = fs.read(&src).await?;
+ dst_file.write_all(&buffer).await?;
+
+ Ok(RpCopy::default())
+ }
+
+ async fn rename(&self, from: &str, to: &str, _args: OpRename) -> Result<RpRename> {
+ let client = self.connect().await?;
+
+ let mut fs = client.fs();
+ fs.set_cwd(&self.root);
+
+ if is_same_parent(from, to) {
+ fs.rename(from, to).await?;
+ } else {
+ self.copy(from, to, OpCopy::default()).await?;
+ fs.remove_file(from).await?;
+ }
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] Xuanwo commented on pull request #2263: feat(services/sftp): support copy and rename for sftp
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on PR #2263:
URL: https://github.com/apache/incubator-opendal/pull/2263#issuecomment-1549967107
> After some research, I think we may need to give up the sftp copy feature.
They are different things: To make `copy` work, we need both client and server supports this operation. So OpenDAL should:
- Support `copy` if server supports.
- Return a `NotSupported` errro if server doesn't.
--
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 pull request #2263: feat(services/sftp): support copy and rename for sftp
Posted by "silver-ymz (via GitHub)" <gi...@apache.org>.
silver-ymz commented on PR #2263:
URL: https://github.com/apache/incubator-opendal/pull/2263#issuecomment-1550901786
> would like to add this in next PR? This PR is good enough to get merged.
Ok. I think it's fine.
--
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 pull request #2263: feat(services/sftp): support copy and rename for sftp
Posted by "silver-ymz (via GitHub)" <gi...@apache.org>.
silver-ymz commented on PR #2263:
URL: https://github.com/apache/incubator-opendal/pull/2263#issuecomment-1548937885
I find the reason. The buffer's length in `TokioCompatFile` of `openssh-sftp-client` is 4096 as default. If we seek the position far than `Current(4096)`, it will only seek to `Current(4096)`. Should we change the impl of `AsyncSeek` in `SftpReaderInner`?
--
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 #2263: feat(services/sftp): support copy and rename for sftp
Posted by "suyanhanx (via GitHub)" <gi...@apache.org>.
suyanhanx commented on PR #2263:
URL: https://github.com/apache/incubator-opendal/pull/2263#issuecomment-1547873801
> There is a new bug about `range_read`.
>
> In https://github.com/apache/incubator-opendal/actions/runs/4976970726/jobs/8905529556, test `write_test_fuzz_range_reader` failed.
> ```
> thread 'services_sftp::write_test_fuzz_range_reader' panicked at 'assertion failed: `(left == right)`
> left: `546602`,
> right: `543843`: check seek failed: output pos is different with expected pos, actions: [Read(84819), Seek(End(-519436)), Seek(Start(346471)), Next, Read(27351), Seek(Start(349302)), Next, Seek(End(-339634)), Next, Read(180661), Seek(End(-561960)), Next, Next, Seek(End(-768061)), Next, Next, Seek(End(-634305)), Next, Next, Read(1337), Seek(End(-328798))]',
> ```
>
> I'm debuging it now.
Is the reason you removed the content-length check?
--
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 pull request #2263: feat(services/sftp): support copy and rename for sftp
Posted by "silver-ymz (via GitHub)" <gi...@apache.org>.
silver-ymz commented on PR #2263:
URL: https://github.com/apache/incubator-opendal/pull/2263#issuecomment-1549971585
> May I ask why we need to detect this? Can we return NotSuppported error while copy failed for server doesn't support it?
This is the way to deal with it now. So should we add copy to `Capability` in `info` function? If so, the tests will fail, because the sftp server in CI doesn't support copy.
--
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 #2263: feat(services/sftp): support copy and rename for sftp
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on PR #2263:
URL: https://github.com/apache/incubator-opendal/pull/2263#issuecomment-1549658424
> I think it isn't a big problem. cc @Xuanwo
No, we can't call async functions in build. And the ability of sftp server should be told by users. I prefer to return not supported instead.
--
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 #2263: feat(services/sftp): support copy and rename for sftp
Posted by "suyanhanx (via GitHub)" <gi...@apache.org>.
suyanhanx commented on code in PR #2263:
URL: https://github.com/apache/incubator-opendal/pull/2263#discussion_r1193331688
##########
core/src/services/sftp/backend.rs:
##########
@@ -375,6 +375,49 @@ impl Accessor for SftpBackend {
Ok((RpWrite::new(), SftpWriter::new(file)))
}
+ async fn copy(&self, from: &str, to: &str, _args: OpCopy) -> Result<RpCopy> {
+ let client = self.connect().await?;
+
+ let mut fs = client.fs();
+ fs.set_cwd(&self.root);
+
+ if let Some((dir, _)) = to.rsplit_once('/') {
+ self.create_dir(dir, OpCreateDir::default()).await?;
+ }
+
+ let src = fs.canonicalize(from).await?;
+ let dst = fs.canonicalize(to).await?;
+ let mut src_file = client.open(&src).await?;
+ let mut dst_file = client.create(dst).await?;
+
+ // if remote sftp server supports copy extension, use it
+ if let Ok(()) = src_file.copy_all_to(&mut dst_file).await {
+ return Ok(RpCopy::default());
+ }
+
+ src_file.close().await?;
+ let buffer = fs.read(&src).await?;
+ dst_file.write_all(&buffer).await?;
+
+ Ok(RpCopy::default())
Review Comment:
We prefer to implement it only when the backend is natively supported.
Return an error then users can handle it themselves. We don't do fallback.
--
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 pull request #2263: feat(services/sftp): support copy and rename for sftp
Posted by "silver-ymz (via GitHub)" <gi...@apache.org>.
silver-ymz commented on PR #2263:
URL: https://github.com/apache/incubator-opendal/pull/2263#issuecomment-1548101750
πAt this time, it passed all tests. Maybe we can open an issue first, and fix it in another 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 pull request #2263: feat(services/sftp): support copy and rename for sftp
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on PR #2263:
URL: https://github.com/apache/incubator-opendal/pull/2263#issuecomment-1549973786
> If so, the tests will fail, because the sftp server in CI doesn't support copy.
We can add a new config called `enable_copy`: users should decide 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] silver-ymz commented on pull request #2263: feat(services/sftp): support copy and rename for sftp
Posted by "silver-ymz (via GitHub)" <gi...@apache.org>.
silver-ymz commented on PR #2263:
URL: https://github.com/apache/incubator-opendal/pull/2263#issuecomment-1549901135
> Auxiliary contains [all supported extensios](https://github.com/openssh-rust/openssh-sftp-client/blob/ec5789501971ed6a5bd8382c2d02d185a21f9bb0/src/auxiliary.rs#L18), so you could expose a new function in openssh-sftp-client easily.
I'll try it in openssh-sftp-client.
--
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