You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zookeeper.apache.org by hanm <gi...@git.apache.org> on 2016/11/04 22:38:00 UTC
[GitHub] zookeeper pull request #100: ZOOKEEPER-2627:Remove ZRWSERVERFOUND from C cli...
GitHub user hanm opened a pull request:
https://github.com/apache/zookeeper/pull/100
ZOOKEEPER-2627:Remove ZRWSERVERFOUND from C client.
JIRA: https://issues.apache.org/jira/browse/ZOOKEEPER-2627
* Remove ZRWSERVERFOUND from C client to maintain consistency of error codes definition between Java / C client.
* Make C client behavior more conforming with Java client in RO mode by having an explicit RW server address and use that address whenever it's available.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/hanm/zookeeper ZOOKEEPER-2627
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/zookeeper/pull/100.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 #100
----
commit 81a0de2298efdce83061faf6f6a4b0b89e245b64
Author: Michael Han <ha...@cloudera.com>
Date: 2016-11-04T22:34:55Z
ZOOKEEPER-2627:Remove ZRWSERVERFOUND from C client.
----
---
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 #100: ZOOKEEPER-2627:Remove ZRWSERVERFOUND from C client.
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on the issue:
https://github.com/apache/zookeeper/pull/100
@breed @rgs1 Gentle nudge: Is this PR good to merge? I have some other PR to be sent over that depends on this one. Would be good to get this in first.
---
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 #100: ZOOKEEPER-2627:Remove ZRWSERVERFOUND from C client.
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on the issue:
https://github.com/apache/zookeeper/pull/100
@breed Wondering if you have time to look at this PR - in particular if breakage of ABI of C client library is a concern or not.
---
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 #100: ZOOKEEPER-2627:Remove ZRWSERVERFOUND from C cli...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/zookeeper/pull/100
---
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 #100: ZOOKEEPER-2627:Remove ZRWSERVERFOUND from C client.
Posted by breed <gi...@git.apache.org>.
Github user breed commented on the issue:
https://github.com/apache/zookeeper/pull/100
+1 everything looks good from our side. we don't use that error code.
---
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 #100: ZOOKEEPER-2627:Remove ZRWSERVERFOUND from C client.
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on the issue:
https://github.com/apache/zookeeper/pull/100
@rgs1 Sure, no need to rush this in.
---
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 #100: ZOOKEEPER-2627:Remove ZRWSERVERFOUND from C cli...
Posted by rgs1 <gi...@git.apache.org>.
Github user rgs1 commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/100#discussion_r86673245
--- Diff: src/c/src/zookeeper.c ---
@@ -2072,6 +2076,11 @@ static int ping_rw_server(zhandle_t* zh)
out:
close(sock);
+ if (!rc) {
+ addr_rw_server = 0;
+ } else {
+ addr_rw_server = &zh->addr_rw_server;
+ }
--- End diff --
nit:
```
addr_rw_server = rc ? &zh->addr_rw_server : 0;
```
---
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 #100: ZOOKEEPER-2627:Remove ZRWSERVERFOUND from C cli...
Posted by rgs1 <gi...@git.apache.org>.
Github user rgs1 commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/100#discussion_r86673221
--- Diff: src/c/src/zookeeper.c ---
@@ -1702,15 +1703,9 @@ static void handle_error(zhandle_t *zh,int rc)
}
cleanup_bufs(zh,1,rc);
zh->fd = -1;
-
+
--- End diff --
nit: drop the whitespace that was introduced
---
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 #100: ZOOKEEPER-2627:Remove ZRWSERVERFOUND from C client.
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on the issue:
https://github.com/apache/zookeeper/pull/100
cool thanks @breed.
@rgs1 Could you please merge & close this PR?
---
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 #100: ZOOKEEPER-2627:Remove ZRWSERVERFOUND from C client.
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on the issue:
https://github.com/apache/zookeeper/pull/100
Thanks for the review, @rgs1. Patch and PR is updated.
---
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 #100: ZOOKEEPER-2627:Remove ZRWSERVERFOUND from C client.
Posted by rgs1 <gi...@git.apache.org>.
Github user rgs1 commented on the issue:
https://github.com/apache/zookeeper/pull/100
Thanks @hanm ! Lets wait until Monday to see what @breed thinks and then I am happy to merge this. What do you think?
---
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.
---