You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@opendal.apache.org by "Young-Flash (via GitHub)" <gi...@apache.org> on 2023/07/31 09:26:41 UTC
[GitHub] [incubator-opendal] Young-Flash opened a new pull request, #2736: feat(oay): Add `read_dir`
Young-Flash opened a new pull request, #2736:
URL: https://github.com/apache/incubator-opendal/pull/2736
https://github.com/apache/incubator-opendal/issues/2586
--
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] Young-Flash commented on a diff in pull request #2736: feat(oay): Add `read_dir`
Posted by "Young-Flash (via GitHub)" <gi...@apache.org>.
Young-Flash commented on code in PR #2736:
URL: https://github.com/apache/incubator-opendal/pull/2736#discussion_r1280597100
##########
bin/oay/src/services/webdav/webdavfs.rs:
##########
@@ -48,11 +52,27 @@ impl DavFileSystem for WebdavFs {
fn read_dir<'a>(
&'a self,
- _path: &'a dav_server::davpath::DavPath,
+ path: &'a dav_server::davpath::DavPath,
_meta: dav_server::fs::ReadDirMeta,
) -> dav_server::fs::FsFuture<dav_server::fs::FsStream<Box<dyn dav_server::fs::DavDirEntry>>>
{
- todo!()
+ async move {
+ let path = path.as_url_string();
+ let mut lister = self.op.list(path.as_str()).await.unwrap();
Review Comment:
> BTW, how many entry in one page ?
These are service implementation details that users should not be aware of.
just my own curiosity hhh
--
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 #2736: feat(oay): Add `read_dir`
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #2736:
URL: https://github.com/apache/incubator-opendal/pull/2736#discussion_r1281312329
##########
bin/oay/src/services/webdav/webdavfs.rs:
##########
@@ -48,11 +52,27 @@ impl DavFileSystem for WebdavFs {
fn read_dir<'a>(
&'a self,
- _path: &'a dav_server::davpath::DavPath,
+ path: &'a dav_server::davpath::DavPath,
_meta: dav_server::fs::ReadDirMeta,
) -> dav_server::fs::FsFuture<dav_server::fs::FsStream<Box<dyn dav_server::fs::DavDirEntry>>>
{
- todo!()
+ async move {
+ let path = path.as_url_string();
+ let mut lister = self.op.list(path.as_str()).await.unwrap();
Review Comment:
We can have a `DevStream { op: Operator, lister: Lister }`
--
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] Young-Flash commented on a diff in pull request #2736: feat(oay): Add `read_dir`
Posted by "Young-Flash (via GitHub)" <gi...@apache.org>.
Young-Flash commented on code in PR #2736:
URL: https://github.com/apache/incubator-opendal/pull/2736#discussion_r1280617485
##########
bin/oay/src/services/webdav/webdavfs.rs:
##########
@@ -48,11 +52,27 @@ impl DavFileSystem for WebdavFs {
fn read_dir<'a>(
&'a self,
- _path: &'a dav_server::davpath::DavPath,
+ path: &'a dav_server::davpath::DavPath,
_meta: dav_server::fs::ReadDirMeta,
) -> dav_server::fs::FsFuture<dav_server::fs::FsStream<Box<dyn dav_server::fs::DavDirEntry>>>
{
- todo!()
+ async move {
+ let path = path.as_url_string();
+ let mut lister = self.op.list(path.as_str()).await.unwrap();
Review Comment:
`FsStream` is a type, not a trait 🤔
##########
bin/oay/src/services/webdav/webdavfs.rs:
##########
@@ -48,11 +52,27 @@ impl DavFileSystem for WebdavFs {
fn read_dir<'a>(
&'a self,
- _path: &'a dav_server::davpath::DavPath,
+ path: &'a dav_server::davpath::DavPath,
_meta: dav_server::fs::ReadDirMeta,
) -> dav_server::fs::FsFuture<dav_server::fs::FsStream<Box<dyn dav_server::fs::DavDirEntry>>>
{
- todo!()
+ async move {
+ let path = path.as_url_string();
+ let mut lister = self.op.list(path.as_str()).await.unwrap();
Review Comment:
`FsStream` is a type, not a trait 🤔
--
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 #2736: feat(oay): Add `read_dir`
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #2736:
URL: https://github.com/apache/incubator-opendal/pull/2736#discussion_r1280636896
##########
bin/oay/src/services/webdav/webdavfs.rs:
##########
@@ -48,11 +52,27 @@ impl DavFileSystem for WebdavFs {
fn read_dir<'a>(
&'a self,
- _path: &'a dav_server::davpath::DavPath,
+ path: &'a dav_server::davpath::DavPath,
_meta: dav_server::fs::ReadDirMeta,
) -> dav_server::fs::FsFuture<dav_server::fs::FsStream<Box<dyn dav_server::fs::DavDirEntry>>>
{
- todo!()
+ async move {
+ let path = path.as_url_string();
+ let mut lister = self.op.list(path.as_str()).await.unwrap();
Review Comment:
> `FsStream` is a type, not a trait thinking
It's an aliase to:
```rust
pub type FsStream<T> = Pin<Box<dyn Stream<Item = T> + Send>>;
```
We just need to return a `Pin<Box<dyn Stream<Item = T>>>`
##########
bin/oay/src/services/webdav/webdavfs.rs:
##########
@@ -48,11 +52,27 @@ impl DavFileSystem for WebdavFs {
fn read_dir<'a>(
&'a self,
- _path: &'a dav_server::davpath::DavPath,
+ path: &'a dav_server::davpath::DavPath,
_meta: dav_server::fs::ReadDirMeta,
) -> dav_server::fs::FsFuture<dav_server::fs::FsStream<Box<dyn dav_server::fs::DavDirEntry>>>
{
- todo!()
+ async move {
+ let path = path.as_url_string();
+ let mut lister = self.op.list(path.as_str()).await.unwrap();
Review Comment:
> `FsStream` is a type, not a trait thinking
It's an aliase to stream:
```rust
pub type FsStream<T> = Pin<Box<dyn Stream<Item = T> + Send>>;
```
We just need to return a `Pin<Box<dyn Stream<Item = 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] Young-Flash commented on a diff in pull request #2736: feat(oay): Add `read_dir`
Posted by "Young-Flash (via GitHub)" <gi...@apache.org>.
Young-Flash commented on code in PR #2736:
URL: https://github.com/apache/incubator-opendal/pull/2736#discussion_r1281309678
##########
bin/oay/src/services/webdav/webdavfs.rs:
##########
@@ -48,11 +52,27 @@ impl DavFileSystem for WebdavFs {
fn read_dir<'a>(
&'a self,
- _path: &'a dav_server::davpath::DavPath,
+ path: &'a dav_server::davpath::DavPath,
_meta: dav_server::fs::ReadDirMeta,
) -> dav_server::fs::FsFuture<dav_server::fs::FsStream<Box<dyn dav_server::fs::DavDirEntry>>>
{
- todo!()
+ async move {
+ let path = path.as_url_string();
+ let mut lister = self.op.list(path.as_str()).await.unwrap();
Review Comment:
My idea is:
```
fn read_dir<'a>(
&'a self,
path: &'a dav_server::davpath::DavPath,
_meta: dav_server::fs::ReadDirMeta,
) -> dav_server::fs::FsFuture<dav_server::fs::FsStream<Box<dyn dav_server::fs::DavDirEntry>>>
{
async move {
let path = path.as_url_string();
let mut lister = self.op.list(path.as_str()).await.unwrap();
let entry_stream = DevStream(lister).boxed();
Ok(entry_stream)
}
.boxed()
}
struct DevStream(Lister);
impl Stream for DevStream {
type Item = Box<dyn DavDirEntry>;
fn poll_next(self: std::pin::Pin<&mut Self>, cx: &mut std::task::Context<'_>) -> std::task::Poll<Option<Self::Item>> {
// use `Lister` poll_next()
}
}
```
But the problem is `Item` in `Lister` is `Result<Entry>`, we need to make it to a `WebDAVDirEntry`, which need a `op` to construct, but we cannot get `op` in `DevStream::poll_next()`.
Or, can we make a `Metadata` from a `Entry` without `op`? It's required in `WebDAVDirEntry::metadata()`. Any suggestion?
--
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 #2736: feat(oay): Add `read_dir`
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #2736:
URL: https://github.com/apache/incubator-opendal/pull/2736#discussion_r1279045524
##########
bin/oay/src/services/webdav/webdavfs.rs:
##########
@@ -48,11 +52,27 @@ impl DavFileSystem for WebdavFs {
fn read_dir<'a>(
&'a self,
- _path: &'a dav_server::davpath::DavPath,
+ path: &'a dav_server::davpath::DavPath,
_meta: dav_server::fs::ReadDirMeta,
) -> dav_server::fs::FsFuture<dav_server::fs::FsStream<Box<dyn dav_server::fs::DavDirEntry>>>
{
- todo!()
+ async move {
+ let path = path.as_url_string();
+ let mut lister = self.op.list(path.as_str()).await.unwrap();
Review Comment:
Code here seems incorrect. List will list the path from start everytime, and `next_page` only list one page in this dir.
--
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] Young-Flash commented on a diff in pull request #2736: feat(oay): Add `read_dir`
Posted by "Young-Flash (via GitHub)" <gi...@apache.org>.
Young-Flash commented on code in PR #2736:
URL: https://github.com/apache/incubator-opendal/pull/2736#discussion_r1279052792
##########
bin/oay/src/services/webdav/webdavfs.rs:
##########
@@ -48,11 +52,27 @@ impl DavFileSystem for WebdavFs {
fn read_dir<'a>(
&'a self,
- _path: &'a dav_server::davpath::DavPath,
+ path: &'a dav_server::davpath::DavPath,
_meta: dav_server::fs::ReadDirMeta,
) -> dav_server::fs::FsFuture<dav_server::fs::FsStream<Box<dyn dav_server::fs::DavDirEntry>>>
{
- todo!()
+ async move {
+ let path = path.as_url_string();
+ let mut lister = self.op.list(path.as_str()).await.unwrap();
Review Comment:
so here should loop `next_page` untill `has_next` to be false ? BTW, how many entry in one page ?
--
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 #2736: feat(oay): Add `read_dir`
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #2736:
URL: https://github.com/apache/incubator-opendal/pull/2736#discussion_r1280609953
##########
bin/oay/src/services/webdav/webdavfs.rs:
##########
@@ -48,11 +52,27 @@ impl DavFileSystem for WebdavFs {
fn read_dir<'a>(
&'a self,
- _path: &'a dav_server::davpath::DavPath,
+ path: &'a dav_server::davpath::DavPath,
_meta: dav_server::fs::ReadDirMeta,
) -> dav_server::fs::FsFuture<dav_server::fs::FsStream<Box<dyn dav_server::fs::DavDirEntry>>>
{
- todo!()
+ async move {
+ let path = path.as_url_string();
+ let mut lister = self.op.list(path.as_str()).await.unwrap();
Review Comment:
The code logic here will collect all items into vec and than transformed into stream again. How about implement `dav_server::fs::FsStream` directly? We can add a newtype like `DevStream(Lister)` and `impl FsStream for DevStream`.
--
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] Young-Flash commented on a diff in pull request #2736: feat(oay): Add `read_dir`
Posted by "Young-Flash (via GitHub)" <gi...@apache.org>.
Young-Flash commented on code in PR #2736:
URL: https://github.com/apache/incubator-opendal/pull/2736#discussion_r1281309678
##########
bin/oay/src/services/webdav/webdavfs.rs:
##########
@@ -48,11 +52,27 @@ impl DavFileSystem for WebdavFs {
fn read_dir<'a>(
&'a self,
- _path: &'a dav_server::davpath::DavPath,
+ path: &'a dav_server::davpath::DavPath,
_meta: dav_server::fs::ReadDirMeta,
) -> dav_server::fs::FsFuture<dav_server::fs::FsStream<Box<dyn dav_server::fs::DavDirEntry>>>
{
- todo!()
+ async move {
+ let path = path.as_url_string();
+ let mut lister = self.op.list(path.as_str()).await.unwrap();
Review Comment:
My idea is:
```rust
fn read_dir<'a>(
&'a self,
path: &'a dav_server::davpath::DavPath,
_meta: dav_server::fs::ReadDirMeta,
) -> dav_server::fs::FsFuture<dav_server::fs::FsStream<Box<dyn dav_server::fs::DavDirEntry>>>
{
async move {
let path = path.as_url_string();
let mut lister = self.op.list(path.as_str()).await.unwrap();
let entry_stream = DevStream(lister).boxed();
Ok(entry_stream)
}
.boxed()
}
struct DevStream(Lister);
impl Stream for DevStream {
type Item = Box<dyn DavDirEntry>;
fn poll_next(self: std::pin::Pin<&mut Self>, cx: &mut std::task::Context<'_>) -> std::task::Poll<Option<Self::Item>> {
// use `Lister` poll_next()
}
}
```
But the problem is `Item` in `Lister` is `Result<Entry>`, we need to make it to a `WebDAVDirEntry`, which need a `op` to construct, but we cannot get `op` in `DevStream::poll_next()`.
Or, can we make a `Metadata` from a `Entry` without `op`? It's required in `WebDAVDirEntry::metadata()`. Any suggestion?
--
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 #2736: feat(oay): Add `read_dir`
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #2736:
URL: https://github.com/apache/incubator-opendal/pull/2736#discussion_r1279063951
##########
bin/oay/src/services/webdav/webdavfs.rs:
##########
@@ -48,11 +52,27 @@ impl DavFileSystem for WebdavFs {
fn read_dir<'a>(
&'a self,
- _path: &'a dav_server::davpath::DavPath,
+ path: &'a dav_server::davpath::DavPath,
_meta: dav_server::fs::ReadDirMeta,
) -> dav_server::fs::FsFuture<dav_server::fs::FsStream<Box<dyn dav_server::fs::DavDirEntry>>>
{
- todo!()
+ async move {
+ let path = path.as_url_string();
+ let mut lister = self.op.list(path.as_str()).await.unwrap();
Review Comment:
> so here should loop `next_page` untill `has_next` to be false ?
`Lister` itself is a `Stream`, you can use `while Ok(entry) in lister.next().await {}` here.
> BTW, how many entry in one page ?
These are service implementation details that users should not be aware of.
--
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 #2736: feat(oay): Add `read_dir`
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #2736:
URL: https://github.com/apache/incubator-opendal/pull/2736#discussion_r1280606836
##########
bin/oay/src/services/webdav/webdavfs.rs:
##########
@@ -48,11 +52,27 @@ impl DavFileSystem for WebdavFs {
fn read_dir<'a>(
&'a self,
- _path: &'a dav_server::davpath::DavPath,
+ path: &'a dav_server::davpath::DavPath,
_meta: dav_server::fs::ReadDirMeta,
) -> dav_server::fs::FsFuture<dav_server::fs::FsStream<Box<dyn dav_server::fs::DavDirEntry>>>
{
- todo!()
+ async move {
+ let path = path.as_url_string();
+ let mut lister = self.op.list(path.as_str()).await.unwrap();
Review Comment:
> just my own curiosity hhh
It's 1000 for s3, 100 for fs.
--
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] Young-Flash commented on pull request #2736: feat(oay): Add `read_dir`
Posted by "Young-Flash (via GitHub)" <gi...@apache.org>.
Young-Flash commented on PR #2736:
URL: https://github.com/apache/incubator-opendal/pull/2736#issuecomment-1663189754
Thanks for your patient 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