----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3781/#review13395 -----------------------------------------------------------
The patch looks really good. Most of my comments are comments about the format. /src/c/include/zookeeper.h <https://reviews.apache.org/r/3781/#comment28709> Its purpose... /src/c/src/addrvec.c <https://reviews.apache.org/r/3781/#comment28711> Are these reds extra tabs? Can we remove them? /src/c/src/addrvec.c <https://reviews.apache.org/r/3781/#comment28710> Initializing i twice. /src/c/src/zk_adaptor.h <https://reviews.apache.org/r/3781/#comment28712> Adding more red here. /src/c/src/zookeeper.c <https://reviews.apache.org/r/3781/#comment28713> Can we use a label that indicates that this goto is due to an error? /src/c/tests/TestReconfig.cc <https://reviews.apache.org/r/3781/#comment28718> I couldn't figure the conventions being used for the names of methods, and I've noticed that a few method names start with capital. Can we make sure that we are complying with the conventions used for existing code? /src/c/tests/TestReconfig.cc <https://reviews.apache.org/r/3781/#comment28714> its random choices... /src/c/tests/TestReconfig.cc <https://reviews.apache.org/r/3781/#comment28715> More red... /src/c/tests/TestReconfig.cc <https://reviews.apache.org/r/3781/#comment28717> CreateHostList -> createHostList /src/c/tests/TestReconfig.cc <https://reviews.apache.org/r/3781/#comment28719> long line /src/c/tests/TestReconfig.cc <https://reviews.apache.org/r/3781/#comment28720> long line /src/c/tests/TestReconfig.cc <https://reviews.apache.org/r/3781/#comment28716> currently connected /src/c/tests/TestReconfig.cc <https://reviews.apache.org/r/3781/#comment28724> If I understand the test correctly, it doesn't really check if the clients have been redistributed properly. Instead, it only checks if the removed server has no client assigned to it. /src/c/tests/TestReconfig.cc <https://reviews.apache.org/r/3781/#comment28721> redistributed /src/c/tests/TestReconfig.cc <https://reviews.apache.org/r/3781/#comment28722> redistribution /src/c/tests/TestReconfig.cc <https://reviews.apache.org/r/3781/#comment28723> redistributed /src/docs/src/documentation/content/xdocs/zookeeperProgrammers.xml <https://reviews.apache.org/r/3781/#comment28725> A suggestion: can we break this up into multiple paragraphs and perhaps highlight the example by putting it in a separate box? /src/java/main/org/apache/zookeeper/client/StaticHostProvider.java <https://reviews.apache.org/r/3781/#comment28726> This javadoc text is overflowing a bit. /src/java/main/org/apache/zookeeper/client/StaticHostProvider.java <https://reviews.apache.org/r/3781/#comment28727> Some red due to formatting. - fpj On Nov. 6, 2012, 11:40 p.m., Alexander Shraer wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/3781/ > ----------------------------------------------------------- > > (Updated Nov. 6, 2012, 11:40 p.m.) > > > Review request for zookeeper. > > > Description > ------- > > https://issues.apache.org/jira/browse/ZOOKEEPER-1355 > > > Diffs > ----- > > /src/c/Makefile.am 1406178 > /src/c/include/zookeeper.h 1406178 > /src/c/src/addrvec.h PRE-CREATION > /src/c/src/addrvec.c PRE-CREATION > /src/c/src/mt_adaptor.c 1406178 > /src/c/src/st_adaptor.c 1406178 > /src/c/src/zk_adaptor.h 1406178 > /src/c/src/zookeeper.c 1406178 > /src/c/tests/TestReconfig.cc PRE-CREATION > /src/c/tests/TestZookeeperClose.cc 1406178 > /src/c/tests/TestZookeeperInit.cc 1406178 > /src/c/tests/ZKMocks.cc 1406178 > /src/docs/src/documentation/content/xdocs/zookeeperProgrammers.xml 1406178 > /src/java/main/org/apache/zookeeper/ZooKeeper.java 1406178 > /src/java/main/org/apache/zookeeper/client/HostProvider.java 1406178 > /src/java/main/org/apache/zookeeper/client/StaticHostProvider.java 1406178 > /src/java/test/org/apache/zookeeper/test/StaticHostProviderTest.java > 1406178 > > Diff: https://reviews.apache.org/r/3781/diff/ > > > Testing > ------- > > new tests included as part of the patch > > > Thanks, > > Alexander Shraer > >
