You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@opendal.apache.org by "Ji-Xinyou (via GitHub)" <gi...@apache.org> on 2023/04/09 10:43:51 UTC
[GitHub] [incubator-opendal] Ji-Xinyou opened a new pull request, #1890: feat(bindings/c): free, limit operation and tests
Ji-Xinyou opened a new pull request, #1890:
URL: https://github.com/apache/incubator-opendal/pull/1890
* Add the freeing of the operator
* Add the limit (set and get) operations
* Add a test for the limit operation
* Modify the test to free the operator after the test
--
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 #1890: feat(bindings/c): add support for free heap-allocated operator
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo merged PR #1890:
URL: https://github.com/apache/incubator-opendal/pull/1890
--
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] Ji-Xinyou commented on a diff in pull request #1890: feat(bindings/c): free, limit operation and tests
Posted by "Ji-Xinyou (via GitHub)" <gi...@apache.org>.
Ji-Xinyou commented on code in PR #1890:
URL: https://github.com/apache/incubator-opendal/pull/1890#discussion_r1161285233
##########
bindings/c/src/lib.rs:
##########
@@ -72,6 +72,41 @@ pub unsafe extern "C" fn opendal_operator_new(scheme: *const c_char) -> opendal_
opendal_operator_ptr::from(op)
}
+/// Free the allocated operator pointed by [`opendal_operator_ptr`]
+#[no_mangle]
+pub extern "C" fn opendal_operator_free(op_ptr: opendal_operator_ptr) {
+ let _ = unsafe { Box::from_raw(op_ptr.get_ref_mut()) };
+ // dropped
+}
+
+/// Get current operator's limit
+#[no_mangle]
+pub extern "C" fn opendal_operator_limit_get(op_ptr: opendal_operator_ptr) -> usize {
Review Comment:
No problem, then this PR should only be about the `opendal_operator_free`
--
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 #1890: feat(bindings/c): free, limit operation and tests
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #1890:
URL: https://github.com/apache/incubator-opendal/pull/1890#discussion_r1161282044
##########
bindings/c/tests/basicio.c:
##########
Review Comment:
> However, I think we can keep these tests for now to make sure the implementation is correct and migrate to behavior testing in the future.
Not needed.
--
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] PsiACE commented on a diff in pull request #1890: feat(bindings/c): free, limit operation and tests
Posted by "PsiACE (via GitHub)" <gi...@apache.org>.
PsiACE commented on code in PR #1890:
URL: https://github.com/apache/incubator-opendal/pull/1890#discussion_r1161279554
##########
bindings/c/tests/basicio.c:
##########
Review Comment:
However, I think we can keep these tests for now to make sure the implementation is correct and migrate to behavior testing in the future.
--
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 #1890: feat(bindings/c): free, limit operation and tests
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #1890:
URL: https://github.com/apache/incubator-opendal/pull/1890#discussion_r1161271098
##########
bindings/c/tests/basicio.c:
##########
Review Comment:
Remove this test please. At current stage, we only need to implement BDD tests as defined in https://github.com/apache/incubator-opendal/tree/main/bindings/tests
##########
bindings/c/src/lib.rs:
##########
@@ -72,6 +72,41 @@ pub unsafe extern "C" fn opendal_operator_new(scheme: *const c_char) -> opendal_
opendal_operator_ptr::from(op)
}
+/// Free the allocated operator pointed by [`opendal_operator_ptr`]
+#[no_mangle]
+pub extern "C" fn opendal_operator_free(op_ptr: opendal_operator_ptr) {
+ let _ = unsafe { Box::from_raw(op_ptr.get_ref_mut()) };
+ // dropped
+}
+
+/// Get current operator's limit
+#[no_mangle]
+pub extern "C" fn opendal_operator_limit_get(op_ptr: opendal_operator_ptr) -> usize {
Review Comment:
Please don't expose those API before we have a working binding. Keep simple and force.
--
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] Ji-Xinyou commented on a diff in pull request #1890: feat(bindings/c): free, limit operation and tests
Posted by "Ji-Xinyou (via GitHub)" <gi...@apache.org>.
Ji-Xinyou commented on code in PR #1890:
URL: https://github.com/apache/incubator-opendal/pull/1890#discussion_r1161284696
##########
bindings/c/tests/basicio.c:
##########
Review Comment:
> Remove this test please. At current stage, we only need to implement BDD tests as defined in https://github.com/apache/incubator-opendal/tree/main/bindings/tests
So you mean, currently as the BDD tests say
```
Feature: OpenDAL Binding
Scenario: OpenDAL Blocking Operations
Given A new OpenDAL Blocking Operator
When Blocking write path "test" with content "Hello, World!"
Then The blocking file "test" should exist
Then The blocking file "test" entry mode must be file
Then The blocking file "test" content length must be 13
Then The blocking file "test" must have content "Hello, World!"
```
We only need to make
1. write/read returns correctly
2. with backend other than `memory`, file `test` should exist
3. the entry mode, content length could be checked by the bindings.
There isn't much features left then, only the `backends` and `entry mode, content length` are not implemented.
So do you mean that, other than things above, i.e. `backends`, `entry mode` and `content length`, we currently only need to polish the other part like error handling?
--
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 #1890: feat(bindings/c): free, limit operation and tests
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #1890:
URL: https://github.com/apache/incubator-opendal/pull/1890#discussion_r1161284840
##########
bindings/c/tests/basicio.c:
##########
Review Comment:
> So do you mean that, other than things above, i.e. `backends`, `entry mode` and `content length`, we currently only need to polish the other part like error handling?
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 a diff in pull request #1890: feat(bindings/c): free, limit operation and tests
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #1890:
URL: https://github.com/apache/incubator-opendal/pull/1890#discussion_r1161282044
##########
bindings/c/tests/basicio.c:
##########
Review Comment:
> However, I think we can keep these tests for now to make sure the implementation is correct and migrate to behavior testing in the future.
Not needed. They are **good to have**, but we should focus on **must to have** jobs at current stage.
--
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] PsiACE commented on a diff in pull request #1890: feat(bindings/c): free, limit operation and tests
Posted by "PsiACE (via GitHub)" <gi...@apache.org>.
PsiACE commented on code in PR #1890:
URL: https://github.com/apache/incubator-opendal/pull/1890#discussion_r1161279189
##########
bindings/c/tests/basicio.c:
##########
Review Comment:
Although it seems to have some complexity, maybe we need to try to test it using https://github.com/cucumber/cucumber-cpp
--
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 #1890: feat(bindings/c): free, limit operation and tests
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #1890:
URL: https://github.com/apache/incubator-opendal/pull/1890#discussion_r1161282044
##########
bindings/c/tests/basicio.c:
##########
Review Comment:
> However, I think we can keep these tests for now to make sure the implementation is correct and migrate to behavior testing in the future.
Not needed. They are **good to have**, and we should focus on **must to have** jobs at current stage.
--
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