You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zookeeper.apache.org by breed <gi...@git.apache.org> on 2016/10/20 18:05:14 UTC
[GitHub] zookeeper pull request #90: ZOOKEEPER-761: Remove *synchronous* calls from t...
GitHub user breed opened a pull request:
https://github.com/apache/zookeeper/pull/90
ZOOKEEPER-761: Remove *synchronous* calls from the *single-threaded* C client API
the synchronous calls from a single-threaded client do not work. this patch
makes using them in a single-threaded client a compilation error.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/breed/zookeeper ZOOKEEPER-761
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/zookeeper/pull/90.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #90
----
commit af047777dd6fc0b74ab723e6ce449c0e80cc73df
Author: Ben Reed <br...@fb.com>
Date: 2016-10-19T17:36:44Z
ZOOKEEPER-761: Remove *synchronous* calls from the *single-threaded* C client API
the synchronous calls from a single-threaded client do not work. this patch
makes using them in a single-threaded client a compilation error.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #90: ZOOKEEPER-761: Remove *synchronous* calls from the *sin...
Posted by fpj <gi...@git.apache.org>.
Github user fpj commented on the issue:
https://github.com/apache/zookeeper/pull/90
It looks good, I just left a comment about creating a jira before we forget. Could you do it before we close this issue, please @breed ?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #90: ZOOKEEPER-761: Remove *synchronous* calls from t...
Posted by breed <gi...@git.apache.org>.
Github user breed commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/90#discussion_r92719995
--- Diff: src/c/tests/TestClient.cc ---
@@ -47,6 +47,10 @@ struct buff_struct_2 {
char *buffer;
};
+// TODO(br33d): the vast majority of this test is not usable with single threaded.
--- End diff --
it's more a matter of implementing the tests than refactoring :)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #90: ZOOKEEPER-761: Remove *synchronous* calls from the *sin...
Posted by fpj <gi...@git.apache.org>.
Github user fpj commented on the issue:
https://github.com/apache/zookeeper/pull/90
It sounds like there is a conflict in `TestReconfigServer`, could you resolve it, please @breed ?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #90: ZOOKEEPER-761: Remove *synchronous* calls from the *sin...
Posted by breed <gi...@git.apache.org>.
Github user breed commented on the issue:
https://github.com/apache/zookeeper/pull/90
@fpj on the plus side that failure validates that the change is correct :) (it is trying to use a sync API with a non threaded test)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #90: ZOOKEEPER-761: Remove *synchronous* calls from t...
Posted by rgs1 <gi...@git.apache.org>.
Github user rgs1 commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/90#discussion_r88183747
--- Diff: src/c/src/zookeeper.c ---
@@ -4052,6 +3958,76 @@ int zoo_amulti(zhandle_t *zh, int count, const zoo_op_t *ops,
return (rc < 0) ? ZMARSHALLINGERROR : ZOK;
}
+
+int zoo_aremove_watchers(zhandle_t *zh, const char *path, ZooWatcherType wtype,
+ watcher_fn watcher, void *watcherCtx, int local,
+ void_completion_t *completion, const void *data)
+{
+ char *server_path = prepend_string(zh, path);
+ int rc;
+ struct oarchive *oa;
+ struct RequestHeader h = { get_xid(), ZOO_REMOVE_WATCHES };
+ struct RemoveWatchesRequest req = { (char*)server_path, wtype };
+ watcher_deregistration_t *wdo;
+
+ if (!zh || !isValidPath(server_path, 0)) {
+ rc = ZBADARGUMENTS;
+ goto done;
+ }
+
+ if (!local && is_unrecoverable(zh)) {
+ rc = ZINVALIDSTATE;
+ goto done;
+ }
+
+ if (!pathHasWatcher(zh, server_path, wtype, watcher, watcherCtx)) {
+ rc = ZNOWATCHER;
+ goto done;
+ }
+
+ if (local) {
+ removeWatchers(zh, server_path, wtype, watcher, watcherCtx);
+#ifdef THREADED
+ notify_sync_completion((struct sync_completion *)data);
--- End diff --
@breed @fpj btw -- sorry for the confusing code. `zoo_aremove_watchers` is sui generis given that it's the only public method that can return `ZOK` without scheduling a remote call (for which then, the callback would be naturally dispatched). Thus, this horrible hack of calling `notify_sync_completion()`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #90: ZOOKEEPER-761: Remove *synchronous* calls from the *sin...
Posted by fpj <gi...@git.apache.org>.
Github user fpj commented on the issue:
https://github.com/apache/zookeeper/pull/90
@breed `make check` does not compile for me:
```
g++ -DHAVE_CONFIG_H -I. -I./include -I./tests -I./generated -DUSE_STATIC_LIB -DZKSERVER_CMD="\"./tests/zkServer.sh\"" -DZOO_IPV6_ENABLED -g -O2 -MT zktest_st-TestReconfigServer.o -MD -MP -MF .deps/zktest_st-TestReconfigServer.Tpo -c -o zktest_st-TestReconfigServer.o `test -f 'tests/TestReconfigServer.cc' || echo './'`tests/TestReconfigServer.cc
In file included from /usr/include/cppunit/TestCase.h:6:0,
from /usr/include/cppunit/TestCaller.h:5,
from /usr/include/cppunit/extensions/HelperMacros.h:9,
from tests/TestReconfigServer.cc:18:
tests/TestReconfigServer.cc: In member function 'void TestReconfigServer::testRemoveFollower()':
tests/TestReconfigServer.cc:153:73: error: 'zoo_getconfig' was not declared in this scope
CPPUNIT_ASSERT_EQUAL((int)ZOK, zoo_getconfig(zk, 0, buf, &len, &stat));
^
tests/TestReconfigServer.cc:174:32: error: 'zoo_reconfig' was not declared in this scope
&stat);
^
In file included from /usr/include/cppunit/TestCase.h:6:0,
from /usr/include/cppunit/TestCaller.h:5,
from /usr/include/cppunit/extensions/HelperMacros.h:9,
from tests/TestReconfigServer.cc:18:
tests/TestReconfigServer.cc: In member function 'void TestReconfigServer::testNonIncremental()':
tests/TestReconfigServer.cc:221:73: error: 'zoo_getconfig' was not declared in this scope
CPPUNIT_ASSERT_EQUAL((int)ZOK, zoo_getconfig(zk, 0, buf, &len, &stat));
^
tests/TestReconfigServer.cc:246:32: error: 'zoo_reconfig' was not declared in this scope
&stat);
^
tests/TestReconfigServer.cc: In member function 'void TestReconfigServer::testRemoveConnectedFollower()':
tests/TestReconfigServer.cc:313:72: error: 'zoo_reconfig' was not declared in this scope
zoo_reconfig(zk, NULL, ss.str().c_str(), NULL, -1, buf, &len, &stat);
^
In file included from /usr/include/cppunit/TestCase.h:6:0,
from /usr/include/cppunit/TestCaller.h:5,
from /usr/include/cppunit/extensions/HelperMacros.h:9,
from tests/TestReconfigServer.cc:18:
tests/TestReconfigServer.cc:314:73: error: 'zoo_getconfig' was not declared in this scope
CPPUNIT_ASSERT_EQUAL((int)ZOK, zoo_getconfig(zk, 0, buf, &len, &stat));
```
have you tried running the C tests?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #90: ZOOKEEPER-761: Remove *synchronous* calls from t...
Posted by breed <gi...@git.apache.org>.
Github user breed commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/90#discussion_r87652058
--- Diff: src/c/src/zookeeper.c ---
@@ -4052,6 +3958,76 @@ int zoo_amulti(zhandle_t *zh, int count, const zoo_op_t *ops,
return (rc < 0) ? ZMARSHALLINGERROR : ZOK;
}
+
+int zoo_aremove_watchers(zhandle_t *zh, const char *path, ZooWatcherType wtype,
+ watcher_fn watcher, void *watcherCtx, int local,
+ void_completion_t *completion, const void *data)
+{
+ char *server_path = prepend_string(zh, path);
+ int rc;
+ struct oarchive *oa;
+ struct RequestHeader h = { get_xid(), ZOO_REMOVE_WATCHES };
+ struct RemoveWatchesRequest req = { (char*)server_path, wtype };
+ watcher_deregistration_t *wdo;
+
+ if (!zh || !isValidPath(server_path, 0)) {
+ rc = ZBADARGUMENTS;
+ goto done;
+ }
+
+ if (!local && is_unrecoverable(zh)) {
+ rc = ZINVALIDSTATE;
+ goto done;
+ }
+
+ if (!pathHasWatcher(zh, server_path, wtype, watcher, watcherCtx)) {
+ rc = ZNOWATCHER;
+ goto done;
+ }
+
+ if (local) {
+ removeWatchers(zh, server_path, wtype, watcher, watcherCtx);
+#ifdef THREADED
+ notify_sync_completion((struct sync_completion *)data);
--- End diff --
so, just to be clear. is this change correct? we don't need to call notify_sync_completion in the non-threaded case. right?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #90: ZOOKEEPER-761: Remove *synchronous* calls from t...
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/90#discussion_r84585224
--- Diff: src/c/include/zookeeper.h ---
@@ -2000,6 +2001,7 @@ ZOOAPI int zoo_set_acl(zhandle_t *zh, const char *path, int version,
* \ref zoo_acreate, \ref zoo_adelete, \ref zoo_aset).
*/
ZOOAPI int zoo_multi(zhandle_t *zh, int count, const zoo_op_t *ops, zoo_op_result_t *results);
+#endif
--- End diff --
+1
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---