You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@opendal.apache.org by "erickguan (via GitHub)" <gi...@apache.org> on 2024/10/22 11:36:14 UTC
[PR] feat(services/gdrive): List shows modified timestamp gdrive [opendal]
erickguan opened a new pull request, #5226:
URL: https://github.com/apache/opendal/pull/5226
# Which issue does this PR close?
Part of #4746.
# Are there any user-facing changes?
None.
I am debugging a few tests so testing on CI.
--
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] feat(services/gdrive): List shows modified timestamp gdrive [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on PR #5226:
URL: https://github.com/apache/opendal/pull/5226#issuecomment-2458651711
Also related to https://github.com/apache/opendal/discussions/5287
--
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] feat(services/gdrive): List shows modified timestamp gdrive [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #5226:
URL: https://github.com/apache/opendal/pull/5226#discussion_r1850517565
##########
core/src/layers/complete.rs:
##########
@@ -188,7 +199,7 @@ impl<A: Access> CompleteAccessor<A> {
));
}
- return Ok(RpStat::new(Metadata::new(EntryMode::DIR)));
+ return Ok(RpStat::new(meta));
Review Comment:
Let's keep this change but remove the `capability` related.
Thank you for the catch!
--
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] feat(services/gdrive): List shows modified timestamp gdrive [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #5226:
URL: https://github.com/apache/opendal/pull/5226#discussion_r1848664200
##########
core/src/layers/complete.rs:
##########
@@ -177,8 +177,19 @@ impl<A: Access> CompleteAccessor<A> {
return Ok(RpStat::new(Metadata::new(EntryMode::DIR)));
}
- // Forward to inner if create_dir is supported.
- if path.ends_with('/') && capability.create_dir {
+ if path.ends_with('/')
Review Comment:
Hi, let's keep this file unchanged. The `create_dir` capability check is related to special handling for object storage. I will separate this part to make it clearer.
--
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] feat(services/gdrive): List shows modified timestamp gdrive [opendal]
Posted by "erickguan (via GitHub)" <gi...@apache.org>.
erickguan commented on PR #5226:
URL: https://github.com/apache/opendal/pull/5226#issuecomment-2430054918
I experimented a bit ([patch](https://gist.github.com/erickguan/80bbf2ea82a10c69d260c53f0dd2f97b)) with the async support for listing. Async helps reduce the run time of course. But extending `OpList` is a breaking change. Since OpenDAL has a compat package, I would happily sync with you when you are preparing a breaking release to reduce the amount of work. (no rush in async support)
Some other findings when running behavior tests but not related to the PR:
1. the behavior test fails on my machine but not in CI. I am casually digging the cause.
2. the uuid random generation in behavior tests generate 3 levels of UUID. This will have truly low chance of collision on CI. But I also want to reduce the level when running against local environment.
--
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] feat(services/gdrive): List shows modified timestamp gdrive [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo merged PR #5226:
URL: https://github.com/apache/opendal/pull/5226
--
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] feat(services/gdrive): List shows modified timestamp gdrive [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #5226:
URL: https://github.com/apache/opendal/pull/5226#discussion_r1850516343
##########
core/src/layers/complete.rs:
##########
@@ -177,8 +177,19 @@ impl<A: Access> CompleteAccessor<A> {
return Ok(RpStat::new(Metadata::new(EntryMode::DIR)));
}
- // Forward to inner if create_dir is supported.
- if path.ends_with('/') && capability.create_dir {
+ if path.ends_with('/')
Review Comment:
Oh, I see. Let's return the metadata we fetched from storage directly.
--
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] feat(services/gdrive): List shows modified timestamp gdrive [opendal]
Posted by "erickguan (via GitHub)" <gi...@apache.org>.
erickguan commented on code in PR #5226:
URL: https://github.com/apache/opendal/pull/5226#discussion_r1850176496
##########
core/src/layers/complete.rs:
##########
@@ -177,8 +177,19 @@ impl<A: Access> CompleteAccessor<A> {
return Ok(RpStat::new(Metadata::new(EntryMode::DIR)));
}
- // Forward to inner if create_dir is supported.
- if path.ends_with('/') && capability.create_dir {
+ if path.ends_with('/')
Review Comment:
Here is the problem. The Google Drive service supports `create_dir,` so OpenDAL will remove the metadata from the Google Drive service without this change.
```rust
// Forward to inner if create_dir is supported.
if path.ends_with('/') && capability.create_dir {
let meta = self.inner.stat(path, args).await?.into_metadata();
if meta.is_file() {
return Err(Error::new(
ErrorKind::NotFound,
"stat expected a directory, but found a file",
));
}
return Ok(RpStat::new(Metadata::new(EntryMode::DIR)));
}
```
I read the RFC 3243, and I am skeptical of the change in this PR. I am not familiar with the context so I have a few ideas in mind:
1. Add a comment explaining the hack to clean up later.
2. Maybe extend the complete layer to respond `meta` without creating a new one.
--
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] feat(services/gdrive): List shows modified timestamp gdrive [opendal]
Posted by "erickguan (via GitHub)" <gi...@apache.org>.
erickguan commented on code in PR #5226:
URL: https://github.com/apache/opendal/pull/5226#discussion_r1850749658
##########
core/src/layers/complete.rs:
##########
@@ -177,8 +177,19 @@ impl<A: Access> CompleteAccessor<A> {
return Ok(RpStat::new(Metadata::new(EntryMode::DIR)));
}
- // Forward to inner if create_dir is supported.
- if path.ends_with('/') && capability.create_dir {
+ if path.ends_with('/')
Review Comment:
Ok. Happy to defer this problem to you!
--
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] feat(services/gdrive): List shows modified timestamp gdrive [opendal]
Posted by "erickguan (via GitHub)" <gi...@apache.org>.
erickguan commented on PR #5226:
URL: https://github.com/apache/opendal/pull/5226#issuecomment-2440141729
@Xuanwo Thanks for the pointer. I have found how `PageList` and Google Drive backend work together. But I am stuck with one failure test with minio in the complete layer. Am I on the right direction continuing this PR?
My understanding is the `CompleteLayer` depends on different services behavior. Because you mentioned the metakey work is under way so I try not to change OpenDAL's behaviors. Unfortunately, adding new stat behavior in this layer breaks minio service.
--
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] feat(services/gdrive): List shows modified timestamp gdrive [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #5226:
URL: https://github.com/apache/opendal/pull/5226#discussion_r1811700128
##########
core/src/services/gdrive/lister.rs:
##########
@@ -64,10 +83,32 @@ impl oio::PageList for GdriveLister {
return Ok(());
}
+ let stat_file_metadata = !self
+ .op
+ .metakey()
+ .is_disjoint(Metakey::ContentLength | Metakey::LastModified);
+
// Return self at the first page.
if ctx.token.is_empty() && !ctx.done {
let path = build_rel_path(&self.core.root, &self.path);
- let e = oio::Entry::new(&path, Metadata::new(EntryMode::DIR));
+ let mut metadata = Metadata::new(EntryMode::DIR);
+ if stat_file_metadata {
Review Comment:
Hi, sorry for not making the `Metakey` behavior clearer. (I'm working on this.)
Metakey represents a best effort hint and is not processed server-side. The service merely needs to supply the most comprehensive metadata available during listing.
The `Operator` will call `stat` as required, for example:
https://github.com/apache/opendal/blob/5e074cc49bc7662c790d88fb9be31e7c67899d64/core/src/types/list.rs#L150-L211
--
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] feat(services/gdrive): List shows modified timestamp gdrive [opendal]
Posted by "erickguan (via GitHub)" <gi...@apache.org>.
erickguan commented on PR #5226:
URL: https://github.com/apache/opendal/pull/5226#issuecomment-2460470012
After a closer look, can I pause this PR until your metakey work?
--
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] feat(services/gdrive): List shows modified timestamp gdrive [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #5226:
URL: https://github.com/apache/opendal/pull/5226#discussion_r1828635266
##########
core/src/layers/complete.rs:
##########
@@ -174,7 +174,12 @@ impl<A: Access> CompleteAccessor<A> {
}
if path == "/" {
- return Ok(RpStat::new(Metadata::new(EntryMode::DIR)));
+ let meta = if capability.stat {
+ self.inner.stat(path, args).await?.into_metadata()
Review Comment:
Hi, it's intended that we don't send `stat` for `/`. The service should always make sure `/` exists.
##########
core/src/services/gdrive/lister.rs:
##########
@@ -90,14 +96,30 @@ impl oio::PageList for GdriveLister {
EntryMode::FILE
};
- let root = &self.core.root;
let path = format!("{}{}", &self.path, file.name);
- let normalized_path = build_rel_path(root, &path);
- // Update path cache with list result.
- self.core.path_cache.insert(&path, &file.id).await;
+ // Update path cache with the list result.
+ //
+ // Only cache non-existent entries. When Google Drive converts a format,
+ // for example, Microsoft PowerPoint, there will be two entries.
+ // These two entries share the same file ID.
+ if let Ok(None) = self.core.path_cache.get(&path).await {
+ self.core.path_cache.insert(&path, &file.id).await;
+ }
- let entry = oio::Entry::new(&normalized_path, Metadata::new(file_type));
+ let root = &self.core.root;
+ let normalized_path = build_rel_path(root, &path);
+
+ let mut meta = Metadata::new(file_type);
+ // Resetting the metakey is necessary when mode is `EntryMode::DIR`,
Review Comment:
The underlying cause might be here:
https://github.com/apache/opendal/blob/259a19e66b532a2905bba955749db6b36bf634e9/core/src/types/metadata.rs#L58-L61
It seems reasonable to remove 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
Re: [PR] feat(services/gdrive): List shows modified timestamp gdrive [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on PR #5226:
URL: https://github.com/apache/opendal/pull/5226#issuecomment-2476832970
> Yes, after the metakey is refactored, our work here will be much cleaner.
Hi, you can continue your work 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
Re: [PR] feat(services/gdrive): List shows modified timestamp gdrive [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on PR #5226:
URL: https://github.com/apache/opendal/pull/5226#issuecomment-2476857480
> Thanks for the ping! I'll pick it up and ping you when it's ready.
Thank you for your patience. Looking forward to your 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
Re: [PR] feat(services/gdrive): List shows modified timestamp gdrive [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on PR #5226:
URL: https://github.com/apache/opendal/pull/5226#issuecomment-2461265532
> After a closer look, can I pause this PR until your metakey work?
Yes, after the metakey is refactored, our work here will be much cleaner.
--
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] feat(services/gdrive): List shows modified timestamp gdrive [opendal]
Posted by "erickguan (via GitHub)" <gi...@apache.org>.
erickguan commented on code in PR #5226:
URL: https://github.com/apache/opendal/pull/5226#discussion_r1812481182
##########
core/src/services/gdrive/lister.rs:
##########
@@ -64,10 +83,32 @@ impl oio::PageList for GdriveLister {
return Ok(());
}
+ let stat_file_metadata = !self
+ .op
+ .metakey()
+ .is_disjoint(Metakey::ContentLength | Metakey::LastModified);
+
// Return self at the first page.
if ctx.token.is_empty() && !ctx.done {
let path = build_rel_path(&self.core.root, &self.path);
- let e = oio::Entry::new(&path, Metadata::new(EntryMode::DIR));
+ let mut metadata = Metadata::new(EntryMode::DIR);
+ if stat_file_metadata {
Review Comment:
Thanks for the review. I get a weird Metadata instance problem, that `Gdrivebackend::stat` returns metadata with a timestamp but `Lister::poll_next` gets a result of Metadata without the timestamp.
I will debug it a bit.
--
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] feat(services/gdrive): List shows modified timestamp gdrive [opendal]
Posted by "erickguan (via GitHub)" <gi...@apache.org>.
erickguan commented on PR #5226:
URL: https://github.com/apache/opendal/pull/5226#issuecomment-2459447341
Thanks for tentative checking in with metakey!
--
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] feat(services/gdrive): List shows modified timestamp gdrive [opendal]
Posted by "erickguan (via GitHub)" <gi...@apache.org>.
erickguan commented on PR #5226:
URL: https://github.com/apache/opendal/pull/5226#issuecomment-2476846570
Thanks for the ping! I'll pick it up and ping you when it's ready.
--
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] feat(services/gdrive): List shows modified timestamp gdrive [opendal]
Posted by "erickguan (via GitHub)" <gi...@apache.org>.
erickguan commented on code in PR #5226:
URL: https://github.com/apache/opendal/pull/5226#discussion_r1850176496
##########
core/src/layers/complete.rs:
##########
@@ -177,8 +177,19 @@ impl<A: Access> CompleteAccessor<A> {
return Ok(RpStat::new(Metadata::new(EntryMode::DIR)));
}
- // Forward to inner if create_dir is supported.
- if path.ends_with('/') && capability.create_dir {
+ if path.ends_with('/')
Review Comment:
Here is the problem. The Google Drive service supports `create_dir,` so OpenDAL will remove the metadata without changes.
```rust
// Forward to inner if create_dir is supported.
if path.ends_with('/') && capability.create_dir {
let meta = self.inner.stat(path, args).await?.into_metadata();
if meta.is_file() {
return Err(Error::new(
ErrorKind::NotFound,
"stat expected a directory, but found a file",
));
}
return Ok(RpStat::new(Metadata::new(EntryMode::DIR)));
}
```
I read the RFC 3243, and I am skeptical of the change in this PR. I am not familiar with the context so I have a few ideas in mind:
1. Add a comment explaining the hack to clean up later.
2. Maybe extend the complete layer to respond `meta` without creating a new one.
--
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