Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC-1477: Remove Object Concept #1477

Merged
merged 8 commits into from
Mar 5, 2023
Merged

RFC-1477: Remove Object Concept #1477

merged 8 commits into from
Mar 5, 2023

Conversation

Xuanwo
Copy link
Member

@Xuanwo Xuanwo commented Mar 5, 2023

I hereby agree to the terms of the CLA available at: https://databend.rs/dev/policies/cla/

Summary

RFC: Remove Object Concept

Signed-off-by: Xuanwo <github@xuanwo.io>
Signed-off-by: Xuanwo <github@xuanwo.io>
Signed-off-by: Xuanwo <github@xuanwo.io>
@Xuanwo Xuanwo changed the title RFC: Remove Object Concept RFC-1477: Remove Object Concept Mar 5, 2023
@Xuanwo Xuanwo marked this pull request as ready for review March 5, 2023 07:48
Signed-off-by: Xuanwo <github@xuanwo.io>
Signed-off-by: Xuanwo <github@xuanwo.io>
@Xuanwo Xuanwo requested a review from suyanhanx March 5, 2023 07:58
src/docs/rfcs/1477_remove_object_concept.md Outdated Show resolved Hide resolved
src/docs/rfcs/1477_remove_object_concept.md Outdated Show resolved Hide resolved
src/docs/rfcs/1477_remove_object_concept.md Outdated Show resolved Hide resolved
src/docs/rfcs/1477_remove_object_concept.md Outdated Show resolved Hide resolved
@suyanhanx
Copy link
Member

suyanhanx commented Mar 5, 2023

I think removing Object makes it difficult to accommodate some concepts.

  1. What should be the scan/list result?

  2. The reuse of objects will be gone.

  3. The scope of Operator is too broad, and most methods are more reasonable to be placed on file/path object instances.

  4. We can also provide some APIs on Operator, like blocking, as it can affect every object that is subsequently created.

And we can provide users with some best practices to let them know, for example, that creating objects incurs costs.

@Xuanwo
Copy link
Member Author

Xuanwo commented Mar 5, 2023

  1. What should be the scan/list result?

The scan/list result will be an Entry or BlockingEntry that contains the same fields as Object, but is only used for scan/list entries.

The public API should look like:

impl Entry {
    pub fn mode(&self) -> EntryType;
    pub fn path(&self) -> &str;
    pub async fn stat(&self) -> Result<Metadata>;
    pub async fn metadata(&self, key: impl Into<MetaKey>) -> Result<Metadata>;
    ...
}

The reuse of objects will be gone.

The reuse of objects will be removed since they are only a simple wrapper of Operator and a path. According to user code, almost no one reuses them. Users won't perform different actions over the same object like the following:

o.read()
o.stat()
o.read()

The scope of Operator is too broad, and most methods are more reasonable to be placed on file/path object instances.

As described in our RFC, Object is just a wrapper of Operator and a path. Removing extra abstraction will simplify the usage of OpenDAL's public API.

We can also provide some APIs on Operator, like blocking, as it can affect every object that is subsequently created.

We have a plan to provide a fn blocking(&self) -> BlockingOperator so that our users can:

let bop = op.blocking();
bop.read("path")?

Signed-off-by: Xuanwo <github@xuanwo.io>
Signed-off-by: Xuanwo <github@xuanwo.io>
@suyanhanx
Copy link
Member

suyanhanx commented Mar 5, 2023

The scan/list result will be an Entry or BlockingEntry that contains the same fields as Object, but is only used for scan/list entries.
The reuse of objects will be removed since they are only a simple wrapper of Operator and a path.

So why not just keep Object?
Is it not the same as always creating an object if we remove Object and let users use something like op.read(&path)?

Even renaming Object to Entry is a good idea. 🤣

Removing extra abstraction will simplify the usage of OpenDAL's public API.

IMO, the profits aren't as substantial as anticipated.

We have a plan to provide a fn blocking(&self) -> BlockingOperator

SGTM.

@Xuanwo
Copy link
Member Author

Xuanwo commented Mar 5, 2023

So why not just keep Object?

Do not multiply entities beyond necessity

By removing Object, we removed a core concept that users need to understand. The following code is simple (It's more like the way of std::fs and tokio::fs):

op.read("file").await?;

But this one, user may need to think:

op.object("file").read().await?;

Even renaming Object to Entry is a good idea.

Sadly, no. Object is not the same with Entry like std::fs::File is not the same with std::fs::DirEntry.

@Xuanwo
Copy link
Member Author

Xuanwo commented Mar 5, 2023

One thing that often bothers me is that Object is an abstract concept in the storage system and cannot correspond to a physical entity. It is difficult for me to explain what it is in one sentence to users without storage background.

That why I want to delete it from OpenDAL.

@suyanhanx
Copy link
Member

Sadly, no. Object is not the same with Entry like std::fs::File is not the same with std::fs::DirEntry.

These two comparisons don't seem to be the same thing.

(It's more like the way of std::fs and tokio::fs)

Yes, many fs do like this way.
However, I somewhat accept this RFC now.

Want to see what the community's thoughts are.

@PsiACE
Copy link
Member

PsiACE commented Mar 5, 2023

We can think of Object as an internal concept that users do not need to understand and perceive unless they have to use it.

Copy link
Member

@PsiACE PsiACE left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the whole, I admire this RFC. We can harmonize opendal’s behavior to std::fs, allowing users to utilize it with no extra cost.

Copy link
Member

@suyanhanx suyanhanx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Looking forward to it.

@Xuanwo Xuanwo requested review from ClSlaid and sundy-li March 5, 2023 16:57
Copy link
Contributor

@ClSlaid ClSlaid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A thunder of spring!

@Xuanwo Xuanwo merged commit fdffd88 into main Mar 5, 2023
@Xuanwo Xuanwo deleted the add-rfc-remove-object-concept branch March 5, 2023 17:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants