You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@opendal.apache.org by "unixzii (via GitHub)" <gi...@apache.org> on 2023/06/14 02:03:21 UTC
[GitHub] [incubator-opendal] unixzii opened a new pull request, #2461: feat(core): add strict mode for capability check
unixzii opened a new pull request, #2461:
URL: https://github.com/apache/incubator-opendal/pull/2461
Fixed #1752.
## 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
- [x] Expose APIs for users to enable the strict mode
- [x] Implement the supporting layer
- [ ] Implement checks for all operations
- [ ] Add tests
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] unixzii commented on a diff in pull request #2461: feat(core): add strict mode for capability check
Posted by "unixzii (via GitHub)" <gi...@apache.org>.
unixzii commented on code in PR #2461:
URL: https://github.com/apache/incubator-opendal/pull/2461#discussion_r1228962580
##########
core/src/layers/capability_check.rs:
##########
@@ -0,0 +1,201 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use std::fmt::Debug;
+use std::fmt::Formatter;
+
+use async_trait::async_trait;
+
+use crate::raw::*;
+use crate::types::Error;
+use crate::types::ErrorKind;
+use crate::types::Result;
+use crate::Capability;
+
+trait CheckCapability {
+ // TODO: Should we return the unsupported option in the result?
+ fn check_capability(&self, cap: &Capability) -> bool;
+}
+
+macro_rules! impl_check_fn_item {
+ ($self:ident, $cap:ident @ $opt_ident:ident => $cap_ident:ident, $($rest:tt)*) => {
+ impl_check_fn_item!($self, $cap @ $opt_ident => $cap_ident);
+ impl_check_fn_item!($self, $cap @ $($rest)*);
+ };
+ ($self:ident, $cap:ident @ $opt_ident:ident => $cap_ident:ident) => {
+ let has_opt = $self.$opt_ident().is_some();
+ if has_opt && !$cap.$cap_ident {
+ return false;
+ }
+ };
+ ($self:ident, $cap:ident @) => {};
+}
+
+macro_rules! impl_check_fn {
+ ($op_type:ty { $($body:tt)* }) => {
+ impl CheckCapability for $op_type {
+ fn check_capability(&self, cap: &Capability) -> bool {
+ impl_check_fn_item!(self, cap @ $($body)*);
+ true
+ }
+ }
+ };
+}
+
+impl_check_fn!(OpRead {
Review Comment:
Maybe the macro is less error-prone? And it also save time copying-pasting the boilerplate code. 🤔
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] unixzii commented on pull request #2461: feat(core): add strict mode for capability check
Posted by "unixzii (via GitHub)" <gi...@apache.org>.
unixzii commented on PR #2461:
URL: https://github.com/apache/incubator-opendal/pull/2461#issuecomment-1590445892
> > 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.
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] unixzii commented on a diff in pull request #2461: feat(core): add strict mode for capability check
Posted by "unixzii (via GitHub)" <gi...@apache.org>.
unixzii commented on code in PR #2461:
URL: https://github.com/apache/incubator-opendal/pull/2461#discussion_r1228959319
##########
core/src/types/operator/builder.rs:
##########
@@ -355,6 +355,28 @@ impl<A: Accessor> OperatorBuilder<A> {
}
}
+ /// Enable the strict mode for the built Operator.
+ ///
+ /// In strict mode, Operator will return error when the given
+ /// operation arguments contain options that are unsupported by
+ /// the service. By default (without calling this method), all
+ /// unsupported options are ignored silently.
+ ///
+ /// # Notes
+ ///
+ /// `OperatorBuilder::with_strict()` uses static dispatch which
+ /// is zero cost. `Operator::with_strict()` uses dynamic dispatch
+ /// which has a bit runtime overhead with an extra vtable lookup
+ /// and unable to inline.
+ ///
+ /// It's always recommended to use `OperatorBuilder::with_strict()`.
+ #[must_use]
+ pub fn with_strict(
Review Comment:
What’s your code change suggestion about this aspect?
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] unixzii commented on pull request #2461: feat(core): add capability check
Posted by "unixzii (via GitHub)" <gi...@apache.org>.
unixzii commented on PR #2461:
URL: https://github.com/apache/incubator-opendal/pull/2461#issuecomment-1593115899
> 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.
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] Xuanwo commented on a diff in pull request #2461: feat(core): add strict mode for capability check
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #2461:
URL: https://github.com/apache/incubator-opendal/pull/2461#discussion_r1228965413
##########
core/src/layers/capability_check.rs:
##########
@@ -0,0 +1,201 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use std::fmt::Debug;
+use std::fmt::Formatter;
+
+use async_trait::async_trait;
+
+use crate::raw::*;
+use crate::types::Error;
+use crate::types::ErrorKind;
+use crate::types::Result;
+use crate::Capability;
+
+trait CheckCapability {
+ // TODO: Should we return the unsupported option in the result?
+ fn check_capability(&self, cap: &Capability) -> bool;
+}
+
+macro_rules! impl_check_fn_item {
+ ($self:ident, $cap:ident @ $opt_ident:ident => $cap_ident:ident, $($rest:tt)*) => {
+ impl_check_fn_item!($self, $cap @ $opt_ident => $cap_ident);
+ impl_check_fn_item!($self, $cap @ $($rest)*);
+ };
+ ($self:ident, $cap:ident @ $opt_ident:ident => $cap_ident:ident) => {
+ let has_opt = $self.$opt_ident().is_some();
+ if has_opt && !$cap.$cap_ident {
+ return false;
+ }
+ };
+ ($self:ident, $cap:ident @) => {};
+}
+
+macro_rules! impl_check_fn {
+ ($op_type:ty { $($body:tt)* }) => {
+ impl CheckCapability for $op_type {
+ fn check_capability(&self, cap: &Capability) -> bool {
+ impl_check_fn_item!(self, cap @ $($body)*);
+ true
+ }
+ }
+ };
+}
+
+impl_check_fn!(OpRead {
Review Comment:
There not many args for operations, it's ok to implement by hand. I perfer to expand it or implement as `OpRead.check(&Capability)`.
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] Xuanwo commented on a diff in pull request #2461: feat(core): add capability check
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #2461:
URL: https://github.com/apache/incubator-opendal/pull/2461#discussion_r1230318915
##########
core/tests/behavior/capability.rs:
##########
@@ -0,0 +1,92 @@
+// Licensed to the Apache Software Foundation (ASF) under one
Review Comment:
> I found many tests don’t cover the capability checks. For example, presign tests will check the capability in their preludes, and just skip the test body if it’s not supported.
Yes, there are part of our coming behavior tests refactor.
##########
core/tests/behavior/capability.rs:
##########
@@ -0,0 +1,92 @@
+// Licensed to the Apache Software Foundation (ASF) under one
Review Comment:
> I found many tests don’t cover the capability checks. For example, presign tests will check the capability in their preludes, and just skip the test body if it’s not supported.
Yes, they are part of our coming behavior tests refactor.
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] unixzii commented on pull request #2461: feat(core): add capability check
Posted by "unixzii (via GitHub)" <gi...@apache.org>.
unixzii commented on PR #2461:
URL: https://github.com/apache/incubator-opendal/pull/2461#issuecomment-1591118896
Just updated the PR, it's now much simpler :)
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] Xuanwo merged pull request #2461: feat(core): add capability check
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo merged PR #2461:
URL: https://github.com/apache/incubator-opendal/pull/2461
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] unixzii commented on pull request #2461: feat(core): add strict mode for capability check
Posted by "unixzii (via GitHub)" <gi...@apache.org>.
unixzii commented on PR #2461:
URL: https://github.com/apache/incubator-opendal/pull/2461#issuecomment-1590462871
> > 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.
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] unixzii commented on a diff in pull request #2461: feat(core): add strict mode for capability check
Posted by "unixzii (via GitHub)" <gi...@apache.org>.
unixzii commented on code in PR #2461:
URL: https://github.com/apache/incubator-opendal/pull/2461#discussion_r1228963023
##########
core/src/layers/capability_check.rs:
##########
@@ -0,0 +1,201 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use std::fmt::Debug;
+use std::fmt::Formatter;
+
+use async_trait::async_trait;
+
+use crate::raw::*;
+use crate::types::Error;
+use crate::types::ErrorKind;
+use crate::types::Result;
+use crate::Capability;
+
+trait CheckCapability {
+ // TODO: Should we return the unsupported option in the result?
+ fn check_capability(&self, cap: &Capability) -> bool;
+}
+
+macro_rules! impl_check_fn_item {
+ ($self:ident, $cap:ident @ $opt_ident:ident => $cap_ident:ident, $($rest:tt)*) => {
+ impl_check_fn_item!($self, $cap @ $opt_ident => $cap_ident);
+ impl_check_fn_item!($self, $cap @ $($rest)*);
+ };
+ ($self:ident, $cap:ident @ $opt_ident:ident => $cap_ident:ident) => {
+ let has_opt = $self.$opt_ident().is_some();
+ if has_opt && !$cap.$cap_ident {
+ return false;
+ }
+ };
+ ($self:ident, $cap:ident @) => {};
+}
+
+macro_rules! impl_check_fn {
+ ($op_type:ty { $($body:tt)* }) => {
+ impl CheckCapability for $op_type {
+ fn check_capability(&self, cap: &Capability) -> bool {
+ impl_check_fn_item!(self, cap @ $($body)*);
+ true
+ }
+ }
+ };
+}
+
+impl_check_fn!(OpRead {
Review Comment:
I think I can simplify the macro to make it more clear to read and maintain.
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] Xuanwo commented on a diff in pull request #2461: feat(core): add capability check
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #2461:
URL: https://github.com/apache/incubator-opendal/pull/2461#discussion_r1229846110
##########
core/tests/behavior/capability.rs:
##########
@@ -0,0 +1,92 @@
+// Licensed to the Apache Software Foundation (ASF) under one
Review Comment:
We don't need to add this behavior test since they already been covered by other tests.
##########
core/src/layers/complete.rs:
##########
@@ -152,6 +158,13 @@ impl<A: Accessor> CompleteReaderAccessor<A> {
args: OpRead,
) -> Result<(RpRead, CompleteReader<A, A::Reader>)> {
let capability = self.meta.capability();
+ if !capability.read {
+ return Err(
Review Comment:
We can add a helper function called `new_capability_unsupported_error(op: Operation) -> Error` for short.
##########
core/src/layers/complete.rs:
##########
@@ -152,6 +158,13 @@ impl<A: Accessor> CompleteReaderAccessor<A> {
args: OpRead,
) -> Result<(RpRead, CompleteReader<A, A::Reader>)> {
let capability = self.meta.capability();
+ if !capability.read {
+ return Err(
+ Error::new(ErrorKind::Unsupported, "operation is not supported")
+ .with_operation("read"),
Review Comment:
We can use `Operation::Read` instead.
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] Xuanwo commented on a diff in pull request #2461: feat(core): add strict mode for capability check
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #2461:
URL: https://github.com/apache/incubator-opendal/pull/2461#discussion_r1228965413
##########
core/src/layers/capability_check.rs:
##########
@@ -0,0 +1,201 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use std::fmt::Debug;
+use std::fmt::Formatter;
+
+use async_trait::async_trait;
+
+use crate::raw::*;
+use crate::types::Error;
+use crate::types::ErrorKind;
+use crate::types::Result;
+use crate::Capability;
+
+trait CheckCapability {
+ // TODO: Should we return the unsupported option in the result?
+ fn check_capability(&self, cap: &Capability) -> bool;
+}
+
+macro_rules! impl_check_fn_item {
+ ($self:ident, $cap:ident @ $opt_ident:ident => $cap_ident:ident, $($rest:tt)*) => {
+ impl_check_fn_item!($self, $cap @ $opt_ident => $cap_ident);
+ impl_check_fn_item!($self, $cap @ $($rest)*);
+ };
+ ($self:ident, $cap:ident @ $opt_ident:ident => $cap_ident:ident) => {
+ let has_opt = $self.$opt_ident().is_some();
+ if has_opt && !$cap.$cap_ident {
+ return false;
+ }
+ };
+ ($self:ident, $cap:ident @) => {};
+}
+
+macro_rules! impl_check_fn {
+ ($op_type:ty { $($body:tt)* }) => {
+ impl CheckCapability for $op_type {
+ fn check_capability(&self, cap: &Capability) -> bool {
+ impl_check_fn_item!(self, cap @ $($body)*);
+ true
+ }
+ }
+ };
+}
+
+impl_check_fn!(OpRead {
Review Comment:
There not many args for operations, it's ok to implement by hand. I perfer to expand it to implement as `OpRead.check(&Capability)`.
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] Xuanwo commented on a diff in pull request #2461: feat(core): add strict mode for capability check
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #2461:
URL: https://github.com/apache/incubator-opendal/pull/2461#discussion_r1228965413
##########
core/src/layers/capability_check.rs:
##########
@@ -0,0 +1,201 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use std::fmt::Debug;
+use std::fmt::Formatter;
+
+use async_trait::async_trait;
+
+use crate::raw::*;
+use crate::types::Error;
+use crate::types::ErrorKind;
+use crate::types::Result;
+use crate::Capability;
+
+trait CheckCapability {
+ // TODO: Should we return the unsupported option in the result?
+ fn check_capability(&self, cap: &Capability) -> bool;
+}
+
+macro_rules! impl_check_fn_item {
+ ($self:ident, $cap:ident @ $opt_ident:ident => $cap_ident:ident, $($rest:tt)*) => {
+ impl_check_fn_item!($self, $cap @ $opt_ident => $cap_ident);
+ impl_check_fn_item!($self, $cap @ $($rest)*);
+ };
+ ($self:ident, $cap:ident @ $opt_ident:ident => $cap_ident:ident) => {
+ let has_opt = $self.$opt_ident().is_some();
+ if has_opt && !$cap.$cap_ident {
+ return false;
+ }
+ };
+ ($self:ident, $cap:ident @) => {};
+}
+
+macro_rules! impl_check_fn {
+ ($op_type:ty { $($body:tt)* }) => {
+ impl CheckCapability for $op_type {
+ fn check_capability(&self, cap: &Capability) -> bool {
+ impl_check_fn_item!(self, cap @ $($body)*);
+ true
+ }
+ }
+ };
+}
+
+impl_check_fn!(OpRead {
Review Comment:
There are not many args for operations, it's ok to implement by hand. I perfer to expand it or implement as `OpRead.check(&Capability)`.
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] unixzii commented on pull request #2461: feat(core): add strict mode for capability check
Posted by "unixzii (via GitHub)" <gi...@apache.org>.
unixzii commented on PR #2461:
URL: https://github.com/apache/incubator-opendal/pull/2461#issuecomment-1590452393
> 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.
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] unixzii commented on a diff in pull request #2461: feat(core): add capability check
Posted by "unixzii (via GitHub)" <gi...@apache.org>.
unixzii commented on code in PR #2461:
URL: https://github.com/apache/incubator-opendal/pull/2461#discussion_r1230326326
##########
core/tests/behavior/capability.rs:
##########
@@ -0,0 +1,92 @@
+// Licensed to the Apache Software Foundation (ASF) under one
Review Comment:
That sounds like a good idea.
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] unixzii commented on pull request #2461: feat(core): add capability check
Posted by "unixzii (via GitHub)" <gi...@apache.org>.
unixzii commented on PR #2461:
URL: https://github.com/apache/incubator-opendal/pull/2461#issuecomment-1593162083
There is still something wrong, I'm fixing it.
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] Xuanwo commented on a diff in pull request #2461: feat(core): add strict mode for capability check
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #2461:
URL: https://github.com/apache/incubator-opendal/pull/2461#discussion_r1228949996
##########
core/src/layers/capability_check.rs:
##########
@@ -0,0 +1,201 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use std::fmt::Debug;
+use std::fmt::Formatter;
+
+use async_trait::async_trait;
+
+use crate::raw::*;
+use crate::types::Error;
+use crate::types::ErrorKind;
+use crate::types::Result;
+use crate::Capability;
+
+trait CheckCapability {
Review Comment:
We don't need a new trait for this.
##########
core/src/layers/capability_check.rs:
##########
@@ -0,0 +1,201 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use std::fmt::Debug;
+use std::fmt::Formatter;
+
+use async_trait::async_trait;
+
+use crate::raw::*;
+use crate::types::Error;
+use crate::types::ErrorKind;
+use crate::types::Result;
+use crate::Capability;
+
+trait CheckCapability {
+ // TODO: Should we return the unsupported option in the result?
+ fn check_capability(&self, cap: &Capability) -> bool;
+}
+
+macro_rules! impl_check_fn_item {
+ ($self:ident, $cap:ident @ $opt_ident:ident => $cap_ident:ident, $($rest:tt)*) => {
+ impl_check_fn_item!($self, $cap @ $opt_ident => $cap_ident);
+ impl_check_fn_item!($self, $cap @ $($rest)*);
+ };
+ ($self:ident, $cap:ident @ $opt_ident:ident => $cap_ident:ident) => {
+ let has_opt = $self.$opt_ident().is_some();
+ if has_opt && !$cap.$cap_ident {
+ return false;
+ }
+ };
+ ($self:ident, $cap:ident @) => {};
+}
+
+macro_rules! impl_check_fn {
+ ($op_type:ty { $($body:tt)* }) => {
+ impl CheckCapability for $op_type {
+ fn check_capability(&self, cap: &Capability) -> bool {
+ impl_check_fn_item!(self, cap @ $($body)*);
+ true
+ }
+ }
+ };
+}
+
+impl_check_fn!(OpRead {
Review Comment:
I prefer to expand by hand instead of macro which is easy to maintain.
##########
core/src/types/operator/builder.rs:
##########
@@ -355,6 +355,28 @@ impl<A: Accessor> OperatorBuilder<A> {
}
}
+ /// Enable the strict mode for the built Operator.
+ ///
+ /// In strict mode, Operator will return error when the given
+ /// operation arguments contain options that are unsupported by
+ /// the service. By default (without calling this method), all
+ /// unsupported options are ignored silently.
+ ///
+ /// # Notes
+ ///
+ /// `OperatorBuilder::with_strict()` uses static dispatch which
+ /// is zero cost. `Operator::with_strict()` uses dynamic dispatch
+ /// which has a bit runtime overhead with an extra vtable lookup
+ /// and unable to inline.
+ ///
+ /// It's always recommended to use `OperatorBuilder::with_strict()`.
+ #[must_use]
+ pub fn with_strict(
Review Comment:
Check operation is mandatory, while checking args for the operation is optional. We can implement operation check first.
##########
core/src/layers/capability_check.rs:
##########
@@ -0,0 +1,201 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use std::fmt::Debug;
+use std::fmt::Formatter;
+
+use async_trait::async_trait;
+
+use crate::raw::*;
+use crate::types::Error;
+use crate::types::ErrorKind;
+use crate::types::Result;
+use crate::Capability;
+
+trait CheckCapability {
+ // TODO: Should we return the unsupported option in the result?
+ fn check_capability(&self, cap: &Capability) -> bool;
+}
+
+macro_rules! impl_check_fn_item {
+ ($self:ident, $cap:ident @ $opt_ident:ident => $cap_ident:ident, $($rest:tt)*) => {
+ impl_check_fn_item!($self, $cap @ $opt_ident => $cap_ident);
+ impl_check_fn_item!($self, $cap @ $($rest)*);
+ };
+ ($self:ident, $cap:ident @ $opt_ident:ident => $cap_ident:ident) => {
+ let has_opt = $self.$opt_ident().is_some();
+ if has_opt && !$cap.$cap_ident {
+ return false;
+ }
+ };
+ ($self:ident, $cap:ident @) => {};
+}
+
+macro_rules! impl_check_fn {
+ ($op_type:ty { $($body:tt)* }) => {
+ impl CheckCapability for $op_type {
+ fn check_capability(&self, cap: &Capability) -> bool {
+ impl_check_fn_item!(self, cap @ $($body)*);
+ true
+ }
+ }
+ };
+}
+
+impl_check_fn!(OpRead {
+ if_match => read_with_if_match,
+ if_none_match => read_with_if_none_match,
+ override_cache_control => read_with_override_cache_control,
+ override_content_disposition => read_with_override_content_disposition,
+});
+
+macro_rules! check_capability {
+ ($msg:literal, $self:ident, $args:ident) => {
+ let meta = $self.metadata();
+ if !$args.check_capability(&meta.capability()) {
+ return Err(Error::new(
+ ErrorKind::UnsupportedOption,
+ "args contain unsupported options",
+ )
+ .with_operation($msg));
+ }
+ };
+}
+
+/// Check whether the given operation arguments are supported by the
+/// underlying services.
+///
+/// # Notes
+///
+/// CapabilityCheckLayer is not a public accessible layer that can be
+/// used by external users. OpenDAL will apply it automatically when
+/// the operator is in strict mode.
+pub struct CapabilityCheckLayer;
Review Comment:
We can do this in `CompleleteLayer` instead of adding a new layer.
##########
core/src/types/error.rs:
##########
@@ -53,6 +53,9 @@ pub enum ErrorKind {
Unexpected,
/// Underlying service doesn't support this operation.
Unsupported,
+ /// The given argument contains some options that are not supported by the
+ /// underlying service.
+ UnsupportedOption,
Review Comment:
We don't want to add a new error code.
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] Xuanwo commented on a diff in pull request #2461: feat(core): add strict mode for capability check
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #2461:
URL: https://github.com/apache/incubator-opendal/pull/2461#discussion_r1228975818
##########
core/src/layers/capability_check.rs:
##########
@@ -0,0 +1,201 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use std::fmt::Debug;
+use std::fmt::Formatter;
+
+use async_trait::async_trait;
+
+use crate::raw::*;
+use crate::types::Error;
+use crate::types::ErrorKind;
+use crate::types::Result;
+use crate::Capability;
+
+trait CheckCapability {
+ // TODO: Should we return the unsupported option in the result?
+ fn check_capability(&self, cap: &Capability) -> bool;
+}
+
+macro_rules! impl_check_fn_item {
+ ($self:ident, $cap:ident @ $opt_ident:ident => $cap_ident:ident, $($rest:tt)*) => {
+ impl_check_fn_item!($self, $cap @ $opt_ident => $cap_ident);
+ impl_check_fn_item!($self, $cap @ $($rest)*);
+ };
+ ($self:ident, $cap:ident @ $opt_ident:ident => $cap_ident:ident) => {
+ let has_opt = $self.$opt_ident().is_some();
+ if has_opt && !$cap.$cap_ident {
+ return false;
+ }
+ };
+ ($self:ident, $cap:ident @) => {};
+}
+
+macro_rules! impl_check_fn {
+ ($op_type:ty { $($body:tt)* }) => {
+ impl CheckCapability for $op_type {
+ fn check_capability(&self, cap: &Capability) -> bool {
+ impl_check_fn_item!(self, cap @ $($body)*);
+ true
+ }
+ }
+ };
+}
+
+impl_check_fn!(OpRead {
Review Comment:
> If all op types will have the same check function, I think we should use a trait to abstract it.
No, I don't think so. We only need to add a trait if we want to work with a trait without knowing its actual type. However, all of our logic here always receives an `OpXxx` struct, so we can call its functions directly.
In OpenDAL, we prefer simplicity.
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] Xuanwo commented on a diff in pull request #2461: feat(core): add strict mode for capability check
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #2461:
URL: https://github.com/apache/incubator-opendal/pull/2461#discussion_r1228965822
##########
core/src/types/operator/builder.rs:
##########
@@ -355,6 +355,28 @@ impl<A: Accessor> OperatorBuilder<A> {
}
}
+ /// Enable the strict mode for the built Operator.
+ ///
+ /// In strict mode, Operator will return error when the given
+ /// operation arguments contain options that are unsupported by
+ /// the service. By default (without calling this method), all
+ /// unsupported options are ignored silently.
+ ///
+ /// # Notes
+ ///
+ /// `OperatorBuilder::with_strict()` uses static dispatch which
+ /// is zero cost. `Operator::with_strict()` uses dynamic dispatch
+ /// which has a bit runtime overhead with an extra vtable lookup
+ /// and unable to inline.
+ ///
+ /// It's always recommended to use `OperatorBuilder::with_strict()`.
+ #[must_use]
+ pub fn with_strict(
Review Comment:
We can add check for `cap.read` first and leave the check for `cap.read_with_range` in next PR.
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] unixzii commented on pull request #2461: feat(core): add strict mode for capability check
Posted by "unixzii (via GitHub)" <gi...@apache.org>.
unixzii commented on PR #2461:
URL: https://github.com/apache/incubator-opendal/pull/2461#issuecomment-1590332861
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.
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] unixzii commented on a diff in pull request #2461: feat(core): add strict mode for capability check
Posted by "unixzii (via GitHub)" <gi...@apache.org>.
unixzii commented on code in PR #2461:
URL: https://github.com/apache/incubator-opendal/pull/2461#discussion_r1228979207
##########
core/src/layers/capability_check.rs:
##########
@@ -0,0 +1,201 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use std::fmt::Debug;
+use std::fmt::Formatter;
+
+use async_trait::async_trait;
+
+use crate::raw::*;
+use crate::types::Error;
+use crate::types::ErrorKind;
+use crate::types::Result;
+use crate::Capability;
+
+trait CheckCapability {
+ // TODO: Should we return the unsupported option in the result?
+ fn check_capability(&self, cap: &Capability) -> bool;
+}
+
+macro_rules! impl_check_fn_item {
+ ($self:ident, $cap:ident @ $opt_ident:ident => $cap_ident:ident, $($rest:tt)*) => {
+ impl_check_fn_item!($self, $cap @ $opt_ident => $cap_ident);
+ impl_check_fn_item!($self, $cap @ $($rest)*);
+ };
+ ($self:ident, $cap:ident @ $opt_ident:ident => $cap_ident:ident) => {
+ let has_opt = $self.$opt_ident().is_some();
+ if has_opt && !$cap.$cap_ident {
+ return false;
+ }
+ };
+ ($self:ident, $cap:ident @) => {};
+}
+
+macro_rules! impl_check_fn {
+ ($op_type:ty { $($body:tt)* }) => {
+ impl CheckCapability for $op_type {
+ fn check_capability(&self, cap: &Capability) -> bool {
+ impl_check_fn_item!(self, cap @ $($body)*);
+ true
+ }
+ }
+ };
+}
+
+impl_check_fn!(OpRead {
Review Comment:
Thanks for clarifying. I will crunch the changes soon.
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] Xuanwo commented on a diff in pull request #2461: feat(core): add strict mode for capability check
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #2461:
URL: https://github.com/apache/incubator-opendal/pull/2461#discussion_r1228974699
##########
core/src/types/operator/builder.rs:
##########
@@ -355,6 +355,28 @@ impl<A: Accessor> OperatorBuilder<A> {
}
}
+ /// Enable the strict mode for the built Operator.
+ ///
+ /// In strict mode, Operator will return error when the given
+ /// operation arguments contain options that are unsupported by
+ /// the service. By default (without calling this method), all
+ /// unsupported options are ignored silently.
+ ///
+ /// # Notes
+ ///
+ /// `OperatorBuilder::with_strict()` uses static dispatch which
+ /// is zero cost. `Operator::with_strict()` uses dynamic dispatch
+ /// which has a bit runtime overhead with an extra vtable lookup
+ /// and unable to inline.
+ ///
+ /// It's always recommended to use `OperatorBuilder::with_strict()`.
+ #[must_use]
+ pub fn with_strict(
Review Comment:
Yes.
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] Xuanwo commented on pull request #2461: feat(core): add strict mode for capability check
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on PR #2461:
URL: https://github.com/apache/incubator-opendal/pull/2461#issuecomment-1590461438
> 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`.
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] unixzii commented on a diff in pull request #2461: feat(core): add capability check
Posted by "unixzii (via GitHub)" <gi...@apache.org>.
unixzii commented on code in PR #2461:
URL: https://github.com/apache/incubator-opendal/pull/2461#discussion_r1230315585
##########
core/tests/behavior/capability.rs:
##########
@@ -0,0 +1,92 @@
+// Licensed to the Apache Software Foundation (ASF) under one
Review Comment:
I found many tests don’t cover the capability checks. For example, presign tests will check the capability in their preludes, and just skip the test body if it’s not supported. To ensure the check is working for users, we need to simulate the use cases where users blindly perform some unsupported operations without checking the capability first.
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] Xuanwo commented on a diff in pull request #2461: feat(core): add capability check
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #2461:
URL: https://github.com/apache/incubator-opendal/pull/2461#discussion_r1230319557
##########
core/tests/behavior/capability.rs:
##########
@@ -0,0 +1,92 @@
+// Licensed to the Apache Software Foundation (ASF) under one
Review Comment:
> To ensure the check is working for users, we need to simulate the use cases where users blindly perform some unsupported operations without checking the capability first.
Those logic are so simple and direct that I don't think we should add behavior test for them. We can add some unit tests in `CompleteLayer` instead.
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] Xuanwo commented on pull request #2461: feat(core): add capability check
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on PR #2461:
URL: https://github.com/apache/incubator-opendal/pull/2461#issuecomment-1593086172
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?
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] unixzii commented on a diff in pull request #2461: feat(core): add strict mode for capability check
Posted by "unixzii (via GitHub)" <gi...@apache.org>.
unixzii commented on code in PR #2461:
URL: https://github.com/apache/incubator-opendal/pull/2461#discussion_r1228961366
##########
core/src/layers/capability_check.rs:
##########
@@ -0,0 +1,201 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use std::fmt::Debug;
+use std::fmt::Formatter;
+
+use async_trait::async_trait;
+
+use crate::raw::*;
+use crate::types::Error;
+use crate::types::ErrorKind;
+use crate::types::Result;
+use crate::Capability;
+
+trait CheckCapability {
Review Comment:
The trait is a zero-cost abstraction to implement the check function for all op types. It’s only used in the call-site module. If we implement the check function by hand then we can eliminate it.
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] Xuanwo commented on pull request #2461: feat(core): add strict mode for capability check
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on PR #2461:
URL: https://github.com/apache/incubator-opendal/pull/2461#issuecomment-1590429250
> 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.
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] unixzii commented on a diff in pull request #2461: feat(core): add strict mode for capability check
Posted by "unixzii (via GitHub)" <gi...@apache.org>.
unixzii commented on code in PR #2461:
URL: https://github.com/apache/incubator-opendal/pull/2461#discussion_r1228970021
##########
core/src/layers/capability_check.rs:
##########
@@ -0,0 +1,201 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use std::fmt::Debug;
+use std::fmt::Formatter;
+
+use async_trait::async_trait;
+
+use crate::raw::*;
+use crate::types::Error;
+use crate::types::ErrorKind;
+use crate::types::Result;
+use crate::Capability;
+
+trait CheckCapability {
+ // TODO: Should we return the unsupported option in the result?
+ fn check_capability(&self, cap: &Capability) -> bool;
+}
+
+macro_rules! impl_check_fn_item {
+ ($self:ident, $cap:ident @ $opt_ident:ident => $cap_ident:ident, $($rest:tt)*) => {
+ impl_check_fn_item!($self, $cap @ $opt_ident => $cap_ident);
+ impl_check_fn_item!($self, $cap @ $($rest)*);
+ };
+ ($self:ident, $cap:ident @ $opt_ident:ident => $cap_ident:ident) => {
+ let has_opt = $self.$opt_ident().is_some();
+ if has_opt && !$cap.$cap_ident {
+ return false;
+ }
+ };
+ ($self:ident, $cap:ident @) => {};
+}
+
+macro_rules! impl_check_fn {
+ ($op_type:ty { $($body:tt)* }) => {
+ impl CheckCapability for $op_type {
+ fn check_capability(&self, cap: &Capability) -> bool {
+ impl_check_fn_item!(self, cap @ $($body)*);
+ true
+ }
+ }
+ };
+}
+
+impl_check_fn!(OpRead {
Review Comment:
If all op types will have the same check function, I think we should use a trait to abstract it.
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] Xuanwo commented on a diff in pull request #2461: feat(core): add strict mode for capability check
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #2461:
URL: https://github.com/apache/incubator-opendal/pull/2461#discussion_r1228965822
##########
core/src/types/operator/builder.rs:
##########
@@ -355,6 +355,28 @@ impl<A: Accessor> OperatorBuilder<A> {
}
}
+ /// Enable the strict mode for the built Operator.
+ ///
+ /// In strict mode, Operator will return error when the given
+ /// operation arguments contain options that are unsupported by
+ /// the service. By default (without calling this method), all
+ /// unsupported options are ignored silently.
+ ///
+ /// # Notes
+ ///
+ /// `OperatorBuilder::with_strict()` uses static dispatch which
+ /// is zero cost. `Operator::with_strict()` uses dynamic dispatch
+ /// which has a bit runtime overhead with an extra vtable lookup
+ /// and unable to inline.
+ ///
+ /// It's always recommended to use `OperatorBuilder::with_strict()`.
+ #[must_use]
+ pub fn with_strict(
Review Comment:
We can add check for `cap.read` first and leave the check for `cap.read_with_range` in next PR.
The check for `cap.read` is must have no matter `strict` has been set or not.
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] unixzii commented on a diff in pull request #2461: feat(core): add strict mode for capability check
Posted by "unixzii (via GitHub)" <gi...@apache.org>.
unixzii commented on code in PR #2461:
URL: https://github.com/apache/incubator-opendal/pull/2461#discussion_r1228968791
##########
core/src/types/operator/builder.rs:
##########
@@ -355,6 +355,28 @@ impl<A: Accessor> OperatorBuilder<A> {
}
}
+ /// Enable the strict mode for the built Operator.
+ ///
+ /// In strict mode, Operator will return error when the given
+ /// operation arguments contain options that are unsupported by
+ /// the service. By default (without calling this method), all
+ /// unsupported options are ignored silently.
+ ///
+ /// # Notes
+ ///
+ /// `OperatorBuilder::with_strict()` uses static dispatch which
+ /// is zero cost. `Operator::with_strict()` uses dynamic dispatch
+ /// which has a bit runtime overhead with an extra vtable lookup
+ /// and unable to inline.
+ ///
+ /// It's always recommended to use `OperatorBuilder::with_strict()`.
+ #[must_use]
+ pub fn with_strict(
Review Comment:
Make sense. So in this PR, we won’t actually add strict mode right? Since the check is at operation-level, I think we can remove this API for now.
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] Xuanwo commented on pull request #2461: feat(core): add strict mode for capability check
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on PR #2461:
URL: https://github.com/apache/incubator-opendal/pull/2461#issuecomment-1590448631
> 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`.
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org