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

storage: add traits and a memory implementation #86

Merged
merged 4 commits into from
Nov 10, 2021
Merged

storage: add traits and a memory implementation #86

merged 4 commits into from
Nov 10, 2021

Conversation

huachaohuang
Copy link
Contributor

@huachaohuang huachaohuang commented Nov 9, 2021

Refactor storage interfaces as described in #80.
This PR also provides a memory storage implementation.

More tests may be added later.

Ref: #68

@huachaohuang huachaohuang added this to the Version 0.2 milestone Nov 9, 2021
@zojw zojw mentioned this pull request Nov 9, 2021
@huachaohuang huachaohuang mentioned this pull request Nov 9, 2021
5 tasks
zojw
zojw previously approved these changes Nov 10, 2021
Copy link
Contributor

@zojw zojw left a comment

Choose a reason for hiding this comment

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

LGTM

@zojw
Copy link
Contributor

zojw commented Nov 10, 2021

do we need to present local/remote/external category concepts in code structure?

https://github.com/engula/engula/pull/80/files#diff-1e47e7218df597c79a882aa1c109e42c332234bd8c92e1c493a788f8f64bb328R38-R40

if so do we need to move mem to local/mem and impl s3 as external/s3?

@huachaohuang
Copy link
Contributor Author

do we need to present local/remote/external category concepts in code structure?

https://github.com/engula/engula/pull/80/files#diff-1e47e7218df597c79a882aa1c109e42c332234bd8c92e1c493a788f8f64bb328R38-R40

if so do we need to move mem to local/mem and impl s3 as external/s3?

No, it's not necessary. We can provide some built-in implementations in storage/. Users can also provide their implementations outside.

As for cloud platforms, maybe we can consider adding a platform crate to put vendor-related things together. For example:

src/
  platform/
    aws/
    azure/

@zojw
Copy link
Contributor

zojw commented Nov 10, 2021

do we need to present local/remote/external category concepts in code structure?
https://github.com/engula/engula/pull/80/files#diff-1e47e7218df597c79a882aa1c109e42c332234bd8c92e1c493a788f8f64bb328R38-R40
if so do we need to move mem to local/mem and impl s3 as external/s3?

No, it's not necessary. We can provide some built-in implementations in storage/. Users can also provide their implementations outside.

As for cloud platforms, maybe we can consider adding a platform crate to put vendor-related things together. For example:

src/
  platform/
    aws/
    azure/

ok, I'd like to implement s3 in src/platform/aws/storage after this merged cc #68

@huachaohuang
Copy link
Contributor Author

@zojw I refactored the file layout a little bit. Specifically, I move storage traits to an api/ directory to make things more clear. Would you mind reviewing it again?

@huachaohuang huachaohuang changed the title storage: implement a memory storage storage: add traits and a memory implementation Nov 10, 2021
Copy link
Contributor

@zojw zojw left a comment

Choose a reason for hiding this comment

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

reset LGTM


/// An interface to upload an object.
#[async_trait]
pub trait StorageObjectUploader {
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe this trait is more suitable laid in storage_object.rs~~~?

ditto MemObjectUploader

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But actually storage_object.rs doesn't need this, it is the return type of a StorageBucket method, so I think it should be considered as a part of StorageBucket.

@huachaohuang
Copy link
Contributor Author

@zojw Thanks for your review. You may be also interested in #82, the implementation is very similar to this.

Let's merge this now.

@huachaohuang huachaohuang merged commit 98908e4 into engula:main Nov 10, 2021
@huachaohuang huachaohuang deleted the storage branch November 10, 2021 11:28
pub trait ObjectHandle {}
pub use async_trait::async_trait;

// TODO: use std::stream::Stream instead
Copy link
Contributor

Choose a reason for hiding this comment

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

May we create an issue for it and note how to achieve it? TODO in comments can be always an issue in the project which is more actionable and comment-able.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Make senses, here is the issue #91

Box::new(stream::iter(object_names))
}

async fn upload_object(&self, name: &str) -> Box<dyn StorageObjectUploader> {
Copy link
Contributor

@zojw zojw Nov 17, 2021

Choose a reason for hiding this comment

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

maybe we need to detect the state of the bucket that owned this object?

e.g. two people A and B
---- A: get bucket "bucket-test" that exists and hold MemBucket to next usage
---- B: empty and delete "bucket-test"
---- A: try to upload object into "bucket-test" <=== should return an error, because the object can not download?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it depends. If we mimic file system behaviors on Linux, it is OK to use an opened but deleted directory/file. Once the directory is closed, all data is gone. But I think we should follow the practice of different object storages here.

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

3 participants