Ji-Xinyou commented on code in PR #2779: URL: https://github.com/apache/incubator-opendal/pull/2779#discussion_r1285193569
########## core/src/docs/rfcs/2779_list_with_metakey.md: ########## @@ -0,0 +1,126 @@ +- Proposal Name: `list_with_metakey` +- Start Date: 2023-08-04 +- RFC PR: [apache/incubator-opendal#2779](https://github.com/apache/incubator-opendal/pull/2779) +- Tracking Issue: [apache/incubator-opendal#0000](https://github.com/apache/incubator-opendal/issues/0000) + +# Summary + +Move `Operator` `metadata` API to `list_with().metakey()` to simplify the usage. + +# Motivation + +The current `Entry` metadata API is: + +```rust +use opendal::Entry; +use opendal::Metakey; + +let meta = op + .metadata(&entry, Metakey::ContentLength | Metakey::ContentType) + .await?; +``` + +This API is difficult to understand and rarely used correctly. And in reality, users always fetch the same set of metadata during listing. + +Take one of our users code as an example: + +```rust +let stream = self + .inner + .scan(&path) + .await + .map_err(|err| format_object_store_error(err, &path))?; + +let stream = stream.then(|res| async { + let entry = res.map_err(|err| format_object_store_error(err, ""))?; + let meta = self + .inner + .metadata(&entry, Metakey::ContentLength | Metakey::LastModified) + .await + .map_err(|err| format_object_store_error(err, entry.path()))?; + + Ok(format_object_meta(entry.path(), &meta)) +}); + +Ok(stream.boxed()) +``` + +By moving metadata to `lister`, our user code can be simplified to: + +```rust +let stream = self + .inner + .scan_with(&path) + .metakey(Metakey::ContentLength | Metakey::LastModified) + .await + .map_err(|err| format_object_store_error(err, &path))?; + +let stream = stream.then(|res| async { + let entry = res.map_err(|err| format_object_store_error(err, ""))?; + let meta = entry.into_metadata() + + Ok(format_object_meta(entry.path(), &meta)) +}); + +Ok(stream.boxed()) +``` + +By introducing this change: + +- Users don't need to capture `Operator` in the closure. +- Users don't need to do async call like `metadata()` again. + +If we don't have this change: + +- every place that could receive a `fn()` must use `Fn()` instead which enforce users to have a generic parameter in their code. +- It's harder for other languages binding to implement `op.metadata()` right. + +# Guide-level explanation + +The new API will be: + +```rust +let entries: Vec<Entry> = op + .list_with("dir") + .metakey(Metakey::ContentLength | Metakey::ContentType).await?; + +let meta: &Metadata = entries[0].metadata(); +``` + +Metadata can be queried directly when listing entries via `metadata()`, and later extracted via `into_parts()`. + +# Reference-level explanation + +We will add `metakey` into `OpList`. Underlying services can use those information to try their best to fetch the metadata. + +There are following possibilities: + +- The entry metadata is met: `Lister` return the entry directly +- The entry metadata is not met and not fully filled: `Lister` will try to send `stat` call to fetch the metadata +- The entry metadata is not met and fully filled: `Lister` will return the entry directly. Review Comment: In general, the design LGTM. Whether certain type of Metadata is returned should be checked by the users. But I think for each service we may specify them through the documentation? It could be the case that users want certain type of metadata, but the service does not support that at all(which leads to returning None). I don't think case like this can be avoided on the code level, so the users have to read the storage documentation. WDYT -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
