You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@opendal.apache.org by "ZutJoe (via GitHub)" <gi...@apache.org> on 2023/11/22 11:56:09 UTC
[PR] refactor(services/azfile): Check if dir exists before create [incubator-opendal]
ZutJoe opened a new pull request, #3652:
URL: https://github.com/apache/incubator-opendal/pull/3652
find the api from https://learn.microsoft.com/en-us/rest/api/storageservices/get-directory-properties
refactor #3646
--
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
Re: [PR] refactor(services/azfile): Check if dir exists before create [incubator-opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #3652:
URL: https://github.com/apache/incubator-opendal/pull/3652#discussion_r1401948679
##########
core/src/services/azfile/core.rs:
##########
@@ -421,6 +421,11 @@ impl AzfileCore {
dirs.push_front(p);
}
for dir in dirs {
+ let resp = self.azfile_get_path_properties(dir).await?;
Review Comment:
The order of pathes is crucial. Implementing this way won't achieve our desired goal. Please think about the case `a/b/c` again.
--
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
Re: [PR] refactor(services/azfile): Check if dir exists before create [incubator-opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #3652:
URL: https://github.com/apache/incubator-opendal/pull/3652#discussion_r1402222479
##########
core/src/services/azfile/core.rs:
##########
@@ -420,6 +420,21 @@ impl AzfileCore {
p = get_parent(p);
dirs.push_front(p);
}
+
+ let mut pop_dir_count = dirs.len() as u8;
+ for dir in dirs.iter().rev() {
+ let resp = self.azfile_get_path_properties(dir).await?;
+ if resp.status() == StatusCode::NOT_FOUND {
+ pop_dir_count -= 1;
+ continue;
+ }
+ break;
+ }
+
+ for _ in 0..pop_dir_count {
+ dirs.pop_front();
+ }
+
Review Comment:
We can use dirs.iter().skip() to avoid calling `pop_front()` in a for loop.
--
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
Re: [PR] refactor(services/azfile): Check if dir exists before create [incubator-opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on PR #3652:
URL: https://github.com/apache/incubator-opendal/pull/3652#issuecomment-1823799673
Have fun!
--
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
Re: [PR] refactor(services/azfile): Check if dir exists before create [incubator-opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo merged PR #3652:
URL: https://github.com/apache/incubator-opendal/pull/3652
--
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
Re: [PR] refactor(services/azfile): Check if dir exists before create [incubator-opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #3652:
URL: https://github.com/apache/incubator-opendal/pull/3652#discussion_r1401948679
##########
core/src/services/azfile/core.rs:
##########
@@ -421,6 +421,11 @@ impl AzfileCore {
dirs.push_front(p);
}
for dir in dirs {
+ let resp = self.azfile_get_path_properties(dir).await?;
Review Comment:
The order of pathes is crucial. Implementing this way won't achieve our desired goal.
--
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
Re: [PR] refactor(services/azfile): Check if dir exists before create [incubator-opendal]
Posted by "ZutJoe (via GitHub)" <gi...@apache.org>.
ZutJoe commented on code in PR #3652:
URL: https://github.com/apache/incubator-opendal/pull/3652#discussion_r1401987857
##########
core/src/services/azfile/core.rs:
##########
@@ -421,6 +421,11 @@ impl AzfileCore {
dirs.push_front(p);
}
for dir in dirs {
+ let resp = self.azfile_get_path_properties(dir).await?;
Review Comment:
You are right, check dir does not exist maybe from `a/b/c` to `a` is better
--
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
Re: [PR] refactor(services/azfile): Check if dir exists before create [incubator-opendal]
Posted by "ZutJoe (via GitHub)" <gi...@apache.org>.
ZutJoe commented on PR #3652:
URL: https://github.com/apache/incubator-opendal/pull/3652#issuecomment-1823798664
> Thanks a lot! 多谢！
Thank you for your patience, advice and review
--
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
Re: [PR] refactor(services/azfile): Check if dir exists before create [incubator-opendal]
Posted by "ZutJoe (via GitHub)" <gi...@apache.org>.
ZutJoe commented on code in PR #3652:
URL: https://github.com/apache/incubator-opendal/pull/3652#discussion_r1402234747
##########
core/src/services/azfile/core.rs:
##########
@@ -420,6 +420,21 @@ impl AzfileCore {
p = get_parent(p);
dirs.push_front(p);
}
+
+ let mut pop_dir_count = dirs.len() as u8;
+ for dir in dirs.iter().rev() {
+ let resp = self.azfile_get_path_properties(dir).await?;
+ if resp.status() == StatusCode::NOT_FOUND {
+ pop_dir_count -= 1;
+ continue;
+ }
+ break;
+ }
+
+ for _ in 0..pop_dir_count {
+ dirs.pop_front();
+ }
+
Review Comment:
`let dirs = dirs.iter().skip(pop_dir_count as usize).collect::<Vec<_>>();`
1. use `Vec` or `Vecdeque` ?
2. pop_dir_count type: `u8` or `usize` which is better ?
--
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
Re: [PR] refactor(services/azfile): Check if dir exists before create [incubator-opendal]
Posted by "ZutJoe (via GitHub)" <gi...@apache.org>.
ZutJoe commented on code in PR #3652:
URL: https://github.com/apache/incubator-opendal/pull/3652#discussion_r1402234747
##########
core/src/services/azfile/core.rs:
##########
@@ -420,6 +420,21 @@ impl AzfileCore {
p = get_parent(p);
dirs.push_front(p);
}
+
+ let mut pop_dir_count = dirs.len() as u8;
+ for dir in dirs.iter().rev() {
+ let resp = self.azfile_get_path_properties(dir).await?;
+ if resp.status() == StatusCode::NOT_FOUND {
+ pop_dir_count -= 1;
+ continue;
+ }
+ break;
+ }
+
+ for _ in 0..pop_dir_count {
+ dirs.pop_front();
+ }
+
Review Comment:
`let dirs = dirs.iter().skip(pop_dir_count as usize).collect::<Vec<_>>();`
1. use `Vec` or `Vecdeque` ?
2. `pop_dir_count` type: `u8` or `usize` which is better ?
--
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
Re: [PR] refactor(services/azfile): Check if dir exists before create [incubator-opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #3652:
URL: https://github.com/apache/incubator-opendal/pull/3652#discussion_r1402351665
##########
core/src/services/azfile/core.rs:
##########
@@ -420,6 +420,21 @@ impl AzfileCore {
p = get_parent(p);
dirs.push_front(p);
}
+
+ let mut pop_dir_count = dirs.len() as u8;
+ for dir in dirs.iter().rev() {
+ let resp = self.azfile_get_path_properties(dir).await?;
+ if resp.status() == StatusCode::NOT_FOUND {
+ pop_dir_count -= 1;
+ continue;
+ }
+ break;
+ }
+
+ for _ in 0..pop_dir_count {
+ dirs.pop_front();
+ }
+
Review Comment:
> 1. use `Vec` or `Vecdeque` ?
We don't need to collect, just use it as an iterator.
> 2. `pop_dir_count` type: `u8` or `usize` which is better ?
Use `usize` is good enough.
--
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