Xuanwo commented on code in PR #2094:
URL:
https://github.com/apache/incubator-opendal/pull/2094#discussion_r1174770429
##########
core/src/docs/concepts.rs:
##########
@@ -17,20 +17,26 @@
//! The core concepts of OpenDAL's public API.
//!
-//! OpenDAL provides a unified abstraction for all storage services.
+//! OpenDAL provides a unified abstraction that helps developers access all
storage services.
//!
//! There are two core concepts in OpenDAL:
//!
-//! - [`Builder`]: Build an instance of underlying services.
-//! - [`Operator`]: A bridge between underlying implementation detail and
unified abstraction.
+//! - [`Builder`]: Builder accepts a series of parameters to set up an
instance of underlying services.
+//! You can adjust the behaviour of underlying services with these parameters.
+//! - [`Operator`]: Developer can access underlying storage services with
manipulating one Operator.
+//! The Operator is a delegate for underlying implementation detail, and
provides one unified access interface,
+//! including `read`, `write`, `list` and so on.
+//! Through Operators, OpenDAL standardizes the access methods of different
storage services into access to a file system-like interface.
Review Comment:
Our goal is not to provide a file system-like interface, and we should avoid
giving our users this impression.
##########
core/src/docs/concepts.rs:
##########
@@ -53,15 +61,20 @@
//! ```
//!
//! # Operator
-//!
-//! The [`Operator`] is a bridge between the underlying implementation details
and the unified abstraction. OpenDAL will erase all generic types and higher
abstraction around it.
+//! The [`Operator`] is a delegate for underlying implementation detail, and
provides one unified access interface.
+//! It will hold one reference of Service with its all generic types erased by
OpenDAL, which is the reason why we
+//! say the Operator is the delegate of one Service.
+//! And the Service also has another name called
[`Accessor`][crate::raw::Accessor].
//!
//! ```text
-//! ┌───────────┐ ┌───────────┐ ┌─────────────────┐
-//! │ │ build() │ │ type erase │ │
-//! │ Builder ├──────────►│ Service ├─────────────►│ Operator │
-//! │ │ │ │ │ │
-//! └───────────┘ └───────────┘ └─────────────────┘
+//! ┌─────────────────────────────┐
Review Comment:
`Operator` doesn't accept `Builder`, `OperatorBuilder` does. But we don't
want to expose the `OperatorBuilder` in concept docs here.
##########
core/src/docs/concepts.rs:
##########
@@ -53,15 +61,20 @@
//! ```
//!
//! # Operator
-//!
-//! The [`Operator`] is a bridge between the underlying implementation details
and the unified abstraction. OpenDAL will erase all generic types and higher
abstraction around it.
+//! The [`Operator`] is a delegate for underlying implementation detail, and
provides one unified access interface.
+//! It will hold one reference of Service with its all generic types erased by
OpenDAL, which is the reason why we
+//! say the Operator is the delegate of one Service.
+//! And the Service also has another name called
[`Accessor`][crate::raw::Accessor].
Review Comment:
Service implements `Accessor`, it's not `another name`.
##########
core/src/raw/accessor.rs:
##########
@@ -26,6 +26,16 @@ use crate::*;
/// Underlying trait of all backends for implementors.
///
+/// The actual data access of storage service happens in Accessor layer.
Review Comment:
Nice one!
##########
core/src/types/builder.rs:
##########
@@ -21,16 +21,25 @@ use std::env;
use crate::raw::*;
use crate::*;
-/// Builder is used to build a storage accessor used by [`Operator`].
+/// Builder is used to set up a real underlying service, i.e. storage accessor.
+///
+/// One builder is usually used by [`Operator`] during its initialization.
+/// It can be created by accepting several k-v pairs from one HashMap, one
iterator and specific environment variables.
+///
+/// By default each builder of underlying service must support deriving from
one HashMap.
+/// Besides that, according to the implementation, each builder may have its
own special methods
+/// to control the behavior of initialization of the underlying service.
+/// It often provides semantic interface instead of using one dynamic k-v
strings.
+/// So during the usage, developer needs to read related doc of builder.
///
/// It's recommended to use [`Operator::new`] to avoid use `Builder` trait
directly.
pub trait Builder: Default {
- /// Associated scheme for this builder.
+ /// Associated scheme for this builder. It indicates what underlying
service is.
const SCHEME: Scheme;
/// The accessor that built by this builder.
type Accessor: Accessor;
- /// Construct a builder from given map.
+ /// Construct a builder from given map which contains several parameters
required by underlying service.
Review Comment:
`required` could be confusing.
##########
core/src/types/builder.rs:
##########
@@ -21,16 +21,25 @@ use std::env;
use crate::raw::*;
use crate::*;
-/// Builder is used to build a storage accessor used by [`Operator`].
+/// Builder is used to set up a real underlying service, i.e. storage accessor.
+///
+/// One builder is usually used by [`Operator`] during its initialization.
+/// It can be created by accepting several k-v pairs from one HashMap, one
iterator and specific environment variables.
+///
+/// By default each builder of underlying service must support deriving from
one HashMap.
+/// Besides that, according to the implementation, each builder may have its
own special methods
Review Comment:
Every builder MUST have it's own special methods and `from_map` and other
API will be built upon those methods.
--
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]
