You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@opendal.apache.org by "suyanhanx (via GitHub)" <gi...@apache.org> on 2023/04/03 07:55:52 UTC
[GitHub] [incubator-opendal] suyanhanx opened a new pull request, #1845: fix(core): necessary operations before doing copy
suyanhanx opened a new pull request, #1845:
URL: https://github.com/apache/incubator-opendal/pull/1845
Add necessary operations before doing copy
- ensure source file
- ensure target parent path
--
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 #1845: fix(core): necessary operations before doing copy
Posted by "suyanhanx (via GitHub)" <gi...@apache.org>.
suyanhanx commented on code in PR #1845:
URL: https://github.com/apache/incubator-opendal/pull/1845#discussion_r1155697684
##########
core/src/services/fs/backend.rs:
##########
@@ -443,12 +443,14 @@ impl Accessor for FsBackend {
}
async fn copy(&self, from: &str, to: &str, _args: OpCopy) -> Result<RpCopy> {
- tokio::fs::copy(
- self.root.join(from.trim_end_matches('/')),
- self.root.join(to.trim_end_matches('/')),
- )
- .await
- .map_err(parse_io_error)?;
+ let from = self.root.join(from.trim_end_matches('/'));
+
+ // try to get the metadata of the source file to ensure it exists
Review Comment:
We should confirm the existence of the source file before we ensure target's parent path.
Otherwise it seems a bit confusing that the folder was created when it failed to copy successfully.
(However, there are still cases where the copy may fail, but the creation of parent path caused by an error in the copy itself is acceptable to me.)
--
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 #1845: fix(core): necessary operations before doing copy
Posted by "suyanhanx (via GitHub)" <gi...@apache.org>.
suyanhanx commented on code in PR #1845:
URL: https://github.com/apache/incubator-opendal/pull/1845#discussion_r1155697684
##########
core/src/services/fs/backend.rs:
##########
@@ -443,12 +443,14 @@ impl Accessor for FsBackend {
}
async fn copy(&self, from: &str, to: &str, _args: OpCopy) -> Result<RpCopy> {
- tokio::fs::copy(
- self.root.join(from.trim_end_matches('/')),
- self.root.join(to.trim_end_matches('/')),
- )
- .await
- .map_err(parse_io_error)?;
+ let from = self.root.join(from.trim_end_matches('/'));
+
+ // try to get the metadata of the source file to ensure it exists
Review Comment:
We should confirm the existence of the source file before we ensure target's parent path.
Otherwise it seems a bit confusing that the folder was created when it failed to copy successfully.
(However, there are still cases where the copy may fail, but the creation of parent path caused by an error in the copy itself is acceptable to me.)
But if that's the case, it doesn't seem to be a problem to remove the check. Because if want to copy, it must necessarily create the folder beforehand.
--
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 #1845: fix(core): necessary operations before doing copy
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #1845:
URL: https://github.com/apache/incubator-opendal/pull/1845#discussion_r1155708974
##########
core/src/services/fs/backend.rs:
##########
@@ -443,12 +443,14 @@ impl Accessor for FsBackend {
}
async fn copy(&self, from: &str, to: &str, _args: OpCopy) -> Result<RpCopy> {
- tokio::fs::copy(
- self.root.join(from.trim_end_matches('/')),
- self.root.join(to.trim_end_matches('/')),
- )
- .await
- .map_err(parse_io_error)?;
+ let from = self.root.join(from.trim_end_matches('/'));
+
+ // try to get the metadata of the source file to ensure it exists
Review Comment:
It's not a big issue for me. Either way is fine. Let's merge and gather feedback from users.
--
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 #1845: fix(core): necessary operations before doing copy
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #1845:
URL: https://github.com/apache/incubator-opendal/pull/1845#discussion_r1155687454
##########
core/src/services/fs/backend.rs:
##########
@@ -443,12 +443,14 @@ impl Accessor for FsBackend {
}
async fn copy(&self, from: &str, to: &str, _args: OpCopy) -> Result<RpCopy> {
- tokio::fs::copy(
- self.root.join(from.trim_end_matches('/')),
- self.root.join(to.trim_end_matches('/')),
- )
- .await
- .map_err(parse_io_error)?;
+ let from = self.root.join(from.trim_end_matches('/'));
+
+ // try to get the metadata of the source file to ensure it exists
Review Comment:
Why do we need to check this? Will `copy` already return `not found`?
--
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 #1845: fix(core): necessary operations before doing copy
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on PR #1845:
URL: https://github.com/apache/incubator-opendal/pull/1845#issuecomment-1493983615
Thanks a lot!
--
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 #1845: fix(core): Add checks before doing copy
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo merged PR #1845:
URL: https://github.com/apache/incubator-opendal/pull/1845
--
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 #1845: fix(core): necessary operations before doing copy
Posted by "suyanhanx (via GitHub)" <gi...@apache.org>.
suyanhanx commented on code in PR #1845:
URL: https://github.com/apache/incubator-opendal/pull/1845#discussion_r1155697684
##########
core/src/services/fs/backend.rs:
##########
@@ -443,12 +443,14 @@ impl Accessor for FsBackend {
}
async fn copy(&self, from: &str, to: &str, _args: OpCopy) -> Result<RpCopy> {
- tokio::fs::copy(
- self.root.join(from.trim_end_matches('/')),
- self.root.join(to.trim_end_matches('/')),
- )
- .await
- .map_err(parse_io_error)?;
+ let from = self.root.join(from.trim_end_matches('/'));
+
+ // try to get the metadata of the source file to ensure it exists
Review Comment:
We should confirm the existence of the source file before we ensure target's parent path.
--
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