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

feat(core): add capability check #2461

Merged
merged 9 commits into from
Jun 15, 2023
Merged

feat(core): add capability check #2461

merged 9 commits into from
Jun 15, 2023

Conversation

unixzii
Copy link
Contributor

@unixzii unixzii commented Jun 14, 2023

Fixed #1752.

In this PR, we will only implement the capability check at operation-level.

Implementation Notes

Static dispatching

with_strict method is added on OperatorBuilder type to eliminate dynamic dispatching & extra heap allocations. That will make the built Operator be strict in its whole lifetime, therefore with_strict method is also added on Operator for flexibility.

Dedicated layer for checking

The capability checking is performed in CapabilityCheckLayer to guarantee the single responsibility principle. It's also hard to mix them with any existing layer while allowing the flexible controls.

Manually checking against all ops

The capability metadata is defined separately from ops (in different modules). We have to enumerate all the cases when checking, and map them manually. Fortunately, this is simplified by an internal macro, which can avoid a large amount of redundant code.

Checklist

  • Confirm the design
  • Expose APIs for users to enable the strict mode
  • Implement the supporting layer
  • Implement checks for all operations
  • Add tests

@unixzii
Copy link
Contributor Author

unixzii commented Jun 14, 2023

Hi @Xuanwo, maybe we can have a quick review of the current design, so we can make sure the direction is good to follow when implementing the rest things.

core/src/layers/capability_check.rs Outdated Show resolved Hide resolved
core/src/layers/capability_check.rs Outdated Show resolved Hide resolved
core/src/layers/capability_check.rs Outdated Show resolved Hide resolved
core/src/types/error.rs Outdated Show resolved Hide resolved
core/src/types/operator/builder.rs Outdated Show resolved Hide resolved
@Xuanwo
Copy link
Member

Xuanwo commented Jun 14, 2023

The capability checking is performed in CapabilityCheckLayer to guarantee the single responsibility principle. It's also hard to mix them with any existing layer while allowing the flexible controls.

All those related tasks are handled at CompleteLayer which only used internally.

@unixzii
Copy link
Contributor Author

unixzii commented Jun 14, 2023

The capability checking is performed in CapabilityCheckLayer to guarantee the single responsibility principle. It's also hard to mix them with any existing layer while allowing the flexible controls.

All those related tasks are handled at CompleteLayer which only used internally.

One question for this, how should we pass the strict mode flag to the CompleteLayer? Once Operator is built, its layers are type-erased and no longer accessible from the operator side. Maybe we need to introduce extra heap allocation to share a boolean between operator and layer? Or use mechanism like task local, scoped variables, etc.

@Xuanwo
Copy link
Member

Xuanwo commented Jun 14, 2023

One question for this, how should we pass the strict mode flag to the CompleteLayer?

We can store a boolean strict in Capability which can set by services config like what we does for batch_max_operations in s3.

@unixzii
Copy link
Contributor Author

unixzii commented Jun 14, 2023

We can store a boolean strict in Capability which can set by services config like what we does for batch_max_operations in s3.

Should we expose the configuration to users? Like overriding it when building the operator.

@Xuanwo
Copy link
Member

Xuanwo commented Jun 14, 2023

Should we expose the configuration to users? Like overriding it when building the operator.

I'm a bit hesitant about this. Maybe we can provide a with_strict in opendal::Builder.

@unixzii
Copy link
Contributor Author

unixzii commented Jun 14, 2023

Should we expose the configuration to users? Like overriding it when building the operator.

I'm a bit hesitant about this. Maybe we can provide a with_strict in opendal::Builder.

No worrying about this, we can implement the mandatory operation-level checks first.

@unixzii unixzii changed the title feat(core): add strict mode for capability check feat(core): add capability check Jun 14, 2023
@unixzii unixzii requested a review from Xuanwo June 14, 2023 12:41
@unixzii
Copy link
Contributor Author

unixzii commented Jun 14, 2023

Just updated the PR, it's now much simpler :)

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.

Mostly looks good to me! Only a few minor changes.

core/src/layers/complete.rs Outdated Show resolved Hide resolved
core/tests/behavior/capability.rs Outdated Show resolved Hide resolved
core/src/layers/complete.rs Outdated Show resolved Hide resolved
@Xuanwo
Copy link
Member

Xuanwo commented Jun 15, 2023

Kv and TypedKv's blokcing cap is not set which results in our tests failed: https://github.com/apache/incubator-opendal/blob/main/core/src/raw/adapters/typed_kv/backend.rs#L65

Can you help fix it in this PR?

@unixzii
Copy link
Contributor Author

unixzii commented Jun 15, 2023

Kv and TypedKv's blokcing cap is not set which results in our tests failed: https://github.com/apache/incubator-opendal/blob/main/core/src/raw/adapters/typed_kv/backend.rs#L65

Can you help fix it in this PR?

Yep, bringing up the commits.

@unixzii
Copy link
Contributor Author

unixzii commented Jun 15, 2023

There is still something wrong, I'm fixing it.

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 marked this pull request as ready for review June 15, 2023 14:40
@Xuanwo Xuanwo requested a review from PsiACE as a code owner June 15, 2023 14:40
@Xuanwo Xuanwo merged commit 029852a into apache:main Jun 15, 2023
@PsiACE PsiACE mentioned this pull request Jun 27, 2023
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add capability checks for operations
2 participants