> On Nov. 13, 2012, 5:02 p.m., fpj wrote: > > /src/c/include/zookeeper.h, line 492 > > <https://reviews.apache.org/r/3781/diff/5/?file=186050#file186050line492> > > > > Its purpose...
Fixed. > On Nov. 13, 2012, 5:02 p.m., fpj wrote: > > /src/c/src/addrvec.c, line 43 > > <https://reviews.apache.org/r/3781/diff/5/?file=186052#file186052line43> > > > > Are these reds extra tabs? Can we remove them? These are lines that have only whitespace. I'll remove them. > On Nov. 13, 2012, 5:02 p.m., fpj wrote: > > /src/c/src/addrvec.c, line 211 > > <https://reviews.apache.org/r/3781/diff/5/?file=186052#file186052line211> > > > > Initializing i twice. Fixed. > On Nov. 13, 2012, 5:02 p.m., fpj wrote: > > /src/c/src/zk_adaptor.h, line 191 > > <https://reviews.apache.org/r/3781/diff/5/?file=186055#file186055line191> > > > > Adding more red here. Fixed. > On Nov. 13, 2012, 5:02 p.m., fpj wrote: > > /src/c/src/zookeeper.c, line 794 > > <https://reviews.apache.org/r/3781/diff/5/?file=186056#file186056line794> > > > > Can we use a label that indicates that this goto is due to an error? Good suggestion. Fixed. > On Nov. 13, 2012, 5:02 p.m., fpj wrote: > > /src/c/tests/TestReconfig.cc, line 37 > > <https://reviews.apache.org/r/3781/diff/5/?file=186057#file186057line37> > > > > 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? Yep, I'll update the naming conventions. > On Nov. 13, 2012, 5:02 p.m., fpj wrote: > > /src/c/tests/TestReconfig.cc, line 83 > > <https://reviews.apache.org/r/3781/diff/5/?file=186057#file186057line83> > > > > More red... Fixed. > On Nov. 13, 2012, 5:02 p.m., fpj wrote: > > /src/c/tests/TestReconfig.cc, line 75 > > <https://reviews.apache.org/r/3781/diff/5/?file=186057#file186057line75> > > > > its random choices... Fixed. > On Nov. 13, 2012, 5:02 p.m., fpj wrote: > > /src/c/tests/TestReconfig.cc, line 298 > > <https://reviews.apache.org/r/3781/diff/5/?file=186057#file186057line298> > > > > CreateHostList -> createHostList Fixed... as well as all the other naming conventions. > On Nov. 13, 2012, 5:02 p.m., fpj wrote: > > /src/c/tests/TestReconfig.cc, line 368 > > <https://reviews.apache.org/r/3781/diff/5/?file=186057#file186057line368> > > > > long line Fixed. > On Nov. 13, 2012, 5:02 p.m., fpj wrote: > > /src/c/tests/TestReconfig.cc, line 376 > > <https://reviews.apache.org/r/3781/diff/5/?file=186057#file186057line376> > > > > long line Fixed. > On Nov. 13, 2012, 5:02 p.m., fpj wrote: > > /src/c/tests/TestReconfig.cc, line 411 > > <https://reviews.apache.org/r/3781/diff/5/?file=186057#file186057line411> > > > > currently connected Fixed. > On Nov. 13, 2012, 5:02 p.m., fpj wrote: > > /src/c/tests/TestReconfig.cc, line 539 > > <https://reviews.apache.org/r/3781/diff/5/?file=186057#file186057line539> > > > > 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. Yes, that's correct. > On Nov. 13, 2012, 5:02 p.m., fpj wrote: > > /src/c/tests/TestReconfig.cc, line 540 > > <https://reviews.apache.org/r/3781/diff/5/?file=186057#file186057line540> > > > > redistributed Fixed. > On Nov. 13, 2012, 5:02 p.m., fpj wrote: > > /src/c/tests/TestReconfig.cc, line 543 > > <https://reviews.apache.org/r/3781/diff/5/?file=186057#file186057line543> > > > > redistribution Fixed. > On Nov. 13, 2012, 5:02 p.m., fpj wrote: > > /src/c/tests/TestReconfig.cc, line 547 > > <https://reviews.apache.org/r/3781/diff/5/?file=186057#file186057line547> > > > > redistributed Fixed. > On Nov. 13, 2012, 5:02 p.m., fpj wrote: > > /src/docs/src/documentation/content/xdocs/zookeeperProgrammers.xml, line 544 > > <https://reviews.apache.org/r/3781/diff/5/?file=186061#file186061line544> > > > > A suggestion: can we break this up into multiple paragraphs and perhaps > > highlight the example by putting it in a separate box? Fixed. > On Nov. 13, 2012, 5:02 p.m., fpj wrote: > > /src/java/main/org/apache/zookeeper/client/StaticHostProvider.java, line 129 > > <https://reviews.apache.org/r/3781/diff/5/?file=186064#file186064line129> > > > > This javadoc text is overflowing a bit. Fixed. > On Nov. 13, 2012, 5:02 p.m., fpj wrote: > > /src/java/main/org/apache/zookeeper/client/StaticHostProvider.java, line 167 > > <https://reviews.apache.org/r/3781/diff/5/?file=186064#file186064line167> > > > > Some red due to formatting. Fixed. - Marshall ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3781/#review13395 ----------------------------------------------------------- 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 > >
