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

docs: Unify capabilities list for kv services #2257

Merged
merged 3 commits into from
May 12, 2023
Merged

docs: Unify capabilities list for kv services #2257

merged 3 commits into from
May 12, 2023

Conversation

lqhuang
Copy link
Contributor

@lqhuang lqhuang commented May 12, 2023

In the very first, I want to migrate sled/rocksdb services to typed_kv. But I found typed_kv has its own data schema (metadata + value), it seems would break loading and writing of existed data? Discussion is required here.

Then I found the type crate::Capability doesn't have scan flag, is this intentional result or deliberate design?

Then for this PR,

  1. Add a scan flag to general Capability
  2. Simply reformat capabilities docs of all KV services to the same.

Notes:

  1. cargo test passed locally.
  2. scan flag for those services capable with scan haven't set to true yet.

Further discussions:

  1. Should we have similar typed_kv migration for sled/rocksdb/memorycache/redis
  2. Should we split services into different categories (and project layout): 1. Block Storage 2. Object Storage 3. KV Storage, more over also have different capabilities: 1. BlockCapability; 2. ObjectCapability, 3. KvCapability

This's my first PR for OpenDAL 😀 Please check more in details.

Thanks for your efforts!

Sincerely,
Lanqing

@Xuanwo
Copy link
Member

Xuanwo commented May 12, 2023

Then I found the type crate::Capability doesn't have scan flag, is this intentional result or deliberate design?

Yes, they are by design. We removed scan in previous refactor.

Should we have similar typed_kv migration for sled/rocksdb/memorycache/redis

Not. They can't be migrated to typed_kv since they can't store a typed_kv::Value with zero cost.

Should we split services into different categories (and project layout)

No, we don't do refactor like this kind. Current code structure is clean and easy to understand.

@lqhuang
Copy link
Contributor Author

lqhuang commented May 12, 2023

@Xuanwo Thanks for reply!

I reverted change of scan flag. Now it's just a docs improvement change.

Yes, they are by design. We removed scan in previous refactor.

Any material or discussion about the design?

@lqhuang lqhuang changed the title fix(types/capability): Add scan flag and unify docs for kv capabilities docs: Unify capabilities list for kv services May 12, 2023
@suyanhanx
Copy link
Member

suyanhanx commented May 12, 2023

Thank you for fixing this!
Here are some informations about the relation between scan and list.

#2072

Wish it is help to you.

Copy link
Member

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

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

Thanks!

@Xuanwo Xuanwo merged commit 84b5e94 into apache:main May 12, 2023
@lqhuang lqhuang deleted the add-scan-to-capability branch May 12, 2023 17:22
@Xuanwo Xuanwo mentioned this pull request May 23, 2023
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.

3 participants