[
https://issues.apache.org/jira/browse/ZOOKEEPER-107?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13234179#comment-13234179
]
Marshall McMullen commented on ZOOKEEPER-107:
---------------------------------------------
Alex, below are some comments I have on the C code only.
1. Use C naming conventions. Specifically:
zhandle_t *zh, char **joiningServers, int numJoiners, char **leavingServers,
int numLeaving, char *newMembers, int newMembersLen, int64_t fromConfig, char
*buffer, int* buffer_len
This should be:
zhandle_t *zh, char **joining_servers, int num_joiners, char
**leaving_servers, int num_leaving, char *new_members, int new_membersLen,
int64_t from_config, char *buffer, int* buffer_len
2. The C code tries very hard to keep the line lengths at 80 characters. A lot
of your code does not.
3. I think that joiningServers and leavingServers should be const as certainly
you aren't changing these in the C code. And it wouldn't make sense for the
contract of these functions if you ever needed to. It also makes it easier to
call from C++ code where I can create a vector then pass it's contents into the
C code without having to const-cast things.
4. newMembers is badly named since it's a FILE. Should be e.g.
newMembershipFile or something
5. newMembersLen is also badly named as it's the length of the *filename* not
the length of the new members themselves.
6. fromConfig should have more meaningful name like old_version.
7. I totally don't understand the purpose of buffer and buffer_len arguments.
Why would they ever be anything other than the list I provided? This seems
unecessary. Moreover, I don't see why you'd expose the raw contents of this
buffer to the caller.
8. What's the purpose of the stat argument? Not sure I understand the use case
for why I'd want the stat structure of the underlying /zookeeper/config path.
Or is this just for getting the version of that znode?
9. I find the combination of the non-incremental and incremental arguments into
the same function overly complicated. Specifically if you're giving the
"newMembership" arguments the rest are ignored (or must be NULL) but if you're
in incremental mode, the non-incremental args must be NULL. Easy to get that
wrong or at least be confused by this. I think it'd be cleaner to have a
separate interface for the nonincremental mode... perhaps a zoo_reconfig and a
zoo_reconfig_nonincremental or zoo_reconfig_fromfile or some such.
Since I'm doing a lot of improvements to the C code already, let me know your
thoughts on the above and I'll make the necessary changes.
> Allow dynamic changes to server cluster membership
> --------------------------------------------------
>
> Key: ZOOKEEPER-107
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-107
> Project: ZooKeeper
> Issue Type: Improvement
> Components: server
> Reporter: Patrick Hunt
> Assignee: Alexander Shraer
> Fix For: 3.5.0
>
> Attachments: SimpleAddition.rtf, ZOOKEEPER-107-1-Mar.patch,
> ZOOKEEPER-107-28-Feb.patch, ZOOKEEPER-107-28-Feb.patch,
> ZOOKEEPER-107-29-Feb.patch, zoo_replicated1.cfg, zoo_replicated1.members,
> zookeeper-3.4.0.jar, zookeeper-dev-fatjar.jar,
> zookeeper-reconfig-sep11.patch, zookeeper-reconfig-sep12.patch
>
>
> Currently cluster membership is statically defined, adding/removing hosts
> to/from the server cluster dynamically needs to be supported.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators:
https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
