You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zookeeper.apache.org by Alexander Shraer <sh...@yahoo-inc.com> on 2012/10/11 00:43:51 UTC
Re: Review Request: Add zk.updateServerList(newServerList)
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3781/
-----------------------------------------------------------
(Updated Oct. 10, 2012, 10:43 p.m.)
Review request for zookeeper.
Changes
-------
Changes to C implementation as described on ZK-1355 Jira.
Description
-------
https://issues.apache.org/jira/browse/ZOOKEEPER-1355
Diffs (updated)
-----
/src/c/Makefile.am 1396840
/src/c/include/zookeeper.h 1396840
/src/c/src/mt_adaptor.c 1396840
/src/c/src/st_adaptor.c 1396840
/src/c/src/zk_adaptor.h 1396840
/src/c/src/zookeeper.c 1396840
/src/c/tests/TestZookeeperClose.cc 1396840
/src/c/tests/TestZookeeperInit.cc 1396840
/src/c/tests/ZKMocks.cc 1396840
/src/docs/src/documentation/content/xdocs/zookeeperProgrammers.xml 1396840
/src/java/main/org/apache/zookeeper/ZooKeeper.java 1396840
/src/java/main/org/apache/zookeeper/client/HostProvider.java 1396840
/src/java/main/org/apache/zookeeper/client/StaticHostProvider.java 1396840
/src/java/test/org/apache/zookeeper/server/quorum/Zab1_0Test.java 1396840
/src/java/test/org/apache/zookeeper/test/StaticHostProviderTest.java 1396840
Diff: https://reviews.apache.org/r/3781/diff/
Testing
-------
new tests included as part of the patch
Thanks,
Alexander Shraer
Re: Review Request: Add zk.updateServerList(newServerList)
Posted by Marshall McMullen <ma...@gmail.com>.
> On Oct. 15, 2012, 11:46 p.m., michim wrote:
> > /src/c/src/zookeeper.c, line 435
> > <https://reviews.apache.org/r/3781/diff/4/?file=176409#file176409line435>
> >
> > Do we need to call both srandom and srand48?
>
> Marshall McMullen wrote:
> No, definitely not. Removed call to srand48. Nice catch.
Actually, removing the call to srand48 seems to have caused one of the new reconfig tests to fail as the expected distribution wasn't sufficiently random without it. I don't see any harm in leaving it in there.
- Marshall
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3781/#review12460
-----------------------------------------------------------
On Oct. 13, 2012, 7:56 a.m., Alexander Shraer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3781/
> -----------------------------------------------------------
>
> (Updated Oct. 13, 2012, 7:56 a.m.)
>
>
> Review request for zookeeper.
>
>
> Description
> -------
>
> https://issues.apache.org/jira/browse/ZOOKEEPER-1355
>
>
> Diffs
> -----
>
> /src/c/Makefile.am 1397800
> /src/c/include/zookeeper.h 1397800
> /src/c/src/mt_adaptor.c 1397800
> /src/c/src/st_adaptor.c 1397800
> /src/c/src/zk_adaptor.h 1397800
> /src/c/src/zookeeper.c 1397800
> /src/c/tests/TestZookeeperClose.cc 1397800
> /src/c/tests/TestZookeeperInit.cc 1397800
> /src/c/tests/ZKMocks.cc 1397800
> /src/docs/src/documentation/content/xdocs/zookeeperProgrammers.xml 1397800
> /src/java/main/org/apache/zookeeper/ZooKeeper.java 1397800
> /src/java/main/org/apache/zookeeper/client/HostProvider.java 1397800
> /src/java/main/org/apache/zookeeper/client/StaticHostProvider.java 1397800
> /src/java/test/org/apache/zookeeper/test/StaticHostProviderTest.java 1397800
>
> Diff: https://reviews.apache.org/r/3781/diff/
>
>
> Testing
> -------
>
> new tests included as part of the patch
>
>
> Thanks,
>
> Alexander Shraer
>
>
Re: Review Request: Add zk.updateServerList(newServerList)
Posted by Marshall McMullen <ma...@gmail.com>.
> On Oct. 15, 2012, 11:46 p.m., michim wrote:
> > /src/c/Makefile.am, line 75
> > <https://reviews.apache.org/r/3781/diff/4/?file=176404#file176404line75>
> >
> > nitpick: a trailing space
Fixed.
> On Oct. 15, 2012, 11:46 p.m., michim wrote:
> > /src/c/src/zookeeper.c, line 435
> > <https://reviews.apache.org/r/3781/diff/4/?file=176409#file176409line435>
> >
> > Do we need to call both srandom and srand48?
No, definitely not. Removed call to srand48. Nice catch.
> On Oct. 15, 2012, 11:46 p.m., michim wrote:
> > /src/c/src/zookeeper.c, line 1105
> > <https://reviews.apache.org/r/3781/diff/4/?file=176409#file176409line1105>
> >
> > typo: st to false -> set to false
Fixed.
> On Oct. 15, 2012, 11:46 p.m., michim wrote:
> > /src/c/src/zookeeper.c, line 1572
> > <https://reviews.apache.org/r/3781/diff/4/?file=176409#file176409line1572>
> >
> > Is there a tab character here?
Yes, and there were a few others in the file that I'd incorrectly added (I don't know why my company uses tabs, but I often forget to switch settings when I go work on zookeeper!).
Fixed.
> On Oct. 15, 2012, 11:46 p.m., michim wrote:
> > /src/c/src/zookeeper.c, line 1925
> > <https://reviews.apache.org/r/3781/diff/4/?file=176409#file176409line1925>
> >
> > Was the number 60 chosen arbitrarily? What happens if zh->recv_timeout is less than 60?
Somewhat arbitrarily. It is designed just to prevent *thrashing* so we don't try to reconnect too quickly in the event that we couldn't connect to *any* of the servers in the ensemble.
recv_timeout is measured in milliseconds, so having a value less than 60 seems unrealistic to me.
If you have a better idea here, please let me know :).
> On Oct. 15, 2012, 11:46 p.m., michim wrote:
> > /src/docs/src/documentation/content/xdocs/zookeeperProgrammers.xml, line 561
> > <https://reviews.apache.org/r/3781/diff/4/?file=176413#file176413line561>
> >
> > typo: and not jus -> and not just
fixed.
- Marshall
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3781/#review12460
-----------------------------------------------------------
On Oct. 13, 2012, 7:56 a.m., Alexander Shraer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3781/
> -----------------------------------------------------------
>
> (Updated Oct. 13, 2012, 7:56 a.m.)
>
>
> Review request for zookeeper.
>
>
> Description
> -------
>
> https://issues.apache.org/jira/browse/ZOOKEEPER-1355
>
>
> Diffs
> -----
>
> /src/c/Makefile.am 1397800
> /src/c/include/zookeeper.h 1397800
> /src/c/src/mt_adaptor.c 1397800
> /src/c/src/st_adaptor.c 1397800
> /src/c/src/zk_adaptor.h 1397800
> /src/c/src/zookeeper.c 1397800
> /src/c/tests/TestZookeeperClose.cc 1397800
> /src/c/tests/TestZookeeperInit.cc 1397800
> /src/c/tests/ZKMocks.cc 1397800
> /src/docs/src/documentation/content/xdocs/zookeeperProgrammers.xml 1397800
> /src/java/main/org/apache/zookeeper/ZooKeeper.java 1397800
> /src/java/main/org/apache/zookeeper/client/HostProvider.java 1397800
> /src/java/main/org/apache/zookeeper/client/StaticHostProvider.java 1397800
> /src/java/test/org/apache/zookeeper/test/StaticHostProviderTest.java 1397800
>
> Diff: https://reviews.apache.org/r/3781/diff/
>
>
> Testing
> -------
>
> new tests included as part of the patch
>
>
> Thanks,
>
> Alexander Shraer
>
>
Re: Review Request: Add zk.updateServerList(newServerList)
Posted by Marshall McMullen <ma...@gmail.com>.
Thanks for the review Michi. I'll upload a new patch later today that fixes
your comments.
On Mon, Oct 15, 2012 at 5:46 PM, <mi...@cs.stanford.edu> wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3781/#review12460
> -----------------------------------------------------------
>
>
>
> /src/c/Makefile.am
> <https://reviews.apache.org/r/3781/#comment26456>
>
> nitpick: a trailing space
>
>
>
> /src/c/src/zookeeper.c
> <https://reviews.apache.org/r/3781/#comment26458>
>
> Do we need to call both srandom and srand48?
>
>
>
> /src/c/src/zookeeper.c
> <https://reviews.apache.org/r/3781/#comment26459>
>
> typo: st to false -> set to false
>
>
>
> /src/c/src/zookeeper.c
> <https://reviews.apache.org/r/3781/#comment26460>
>
> Is there a tab character here?
>
>
>
> /src/c/src/zookeeper.c
> <https://reviews.apache.org/r/3781/#comment26462>
>
> Was the number 60 chosen arbitrarily? What happens if zh->recv_timeout
> is less than 60?
>
>
>
> /src/docs/src/documentation/content/xdocs/zookeeperProgrammers.xml
> <https://reviews.apache.org/r/3781/#comment26463>
>
> typo: and not jus -> and not just
>
>
> I reviewed the C client part. Overall, it looks great!
>
> --Michi
>
> - michim
>
>
> On Oct. 13, 2012, 7:56 a.m., Alexander Shraer wrote:
> >
> > -----------------------------------------------------------
> > This is an automatically generated e-mail. To reply, visit:
> > https://reviews.apache.org/r/3781/
> > -----------------------------------------------------------
> >
> > (Updated Oct. 13, 2012, 7:56 a.m.)
> >
> >
> > Review request for zookeeper.
> >
> >
> > Description
> > -------
> >
> > https://issues.apache.org/jira/browse/ZOOKEEPER-1355
> >
> >
> > Diffs
> > -----
> >
> > /src/c/Makefile.am 1397800
> > /src/c/include/zookeeper.h 1397800
> > /src/c/src/mt_adaptor.c 1397800
> > /src/c/src/st_adaptor.c 1397800
> > /src/c/src/zk_adaptor.h 1397800
> > /src/c/src/zookeeper.c 1397800
> > /src/c/tests/TestZookeeperClose.cc 1397800
> > /src/c/tests/TestZookeeperInit.cc 1397800
> > /src/c/tests/ZKMocks.cc 1397800
> > /src/docs/src/documentation/content/xdocs/zookeeperProgrammers.xml
> 1397800
> > /src/java/main/org/apache/zookeeper/ZooKeeper.java 1397800
> > /src/java/main/org/apache/zookeeper/client/HostProvider.java 1397800
> > /src/java/main/org/apache/zookeeper/client/StaticHostProvider.java
> 1397800
> > /src/java/test/org/apache/zookeeper/test/StaticHostProviderTest.java
> 1397800
> >
> > Diff: https://reviews.apache.org/r/3781/diff/
> >
> >
> > Testing
> > -------
> >
> > new tests included as part of the patch
> >
> >
> > Thanks,
> >
> > Alexander Shraer
> >
> >
>
>
Re: Review Request: Add zk.updateServerList(newServerList)
Posted by mi...@cs.stanford.edu.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3781/#review12460
-----------------------------------------------------------
/src/c/Makefile.am
<https://reviews.apache.org/r/3781/#comment26456>
nitpick: a trailing space
/src/c/src/zookeeper.c
<https://reviews.apache.org/r/3781/#comment26458>
Do we need to call both srandom and srand48?
/src/c/src/zookeeper.c
<https://reviews.apache.org/r/3781/#comment26459>
typo: st to false -> set to false
/src/c/src/zookeeper.c
<https://reviews.apache.org/r/3781/#comment26460>
Is there a tab character here?
/src/c/src/zookeeper.c
<https://reviews.apache.org/r/3781/#comment26462>
Was the number 60 chosen arbitrarily? What happens if zh->recv_timeout is less than 60?
/src/docs/src/documentation/content/xdocs/zookeeperProgrammers.xml
<https://reviews.apache.org/r/3781/#comment26463>
typo: and not jus -> and not just
I reviewed the C client part. Overall, it looks great!
--Michi
- michim
On Oct. 13, 2012, 7:56 a.m., Alexander Shraer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3781/
> -----------------------------------------------------------
>
> (Updated Oct. 13, 2012, 7:56 a.m.)
>
>
> Review request for zookeeper.
>
>
> Description
> -------
>
> https://issues.apache.org/jira/browse/ZOOKEEPER-1355
>
>
> Diffs
> -----
>
> /src/c/Makefile.am 1397800
> /src/c/include/zookeeper.h 1397800
> /src/c/src/mt_adaptor.c 1397800
> /src/c/src/st_adaptor.c 1397800
> /src/c/src/zk_adaptor.h 1397800
> /src/c/src/zookeeper.c 1397800
> /src/c/tests/TestZookeeperClose.cc 1397800
> /src/c/tests/TestZookeeperInit.cc 1397800
> /src/c/tests/ZKMocks.cc 1397800
> /src/docs/src/documentation/content/xdocs/zookeeperProgrammers.xml 1397800
> /src/java/main/org/apache/zookeeper/ZooKeeper.java 1397800
> /src/java/main/org/apache/zookeeper/client/HostProvider.java 1397800
> /src/java/main/org/apache/zookeeper/client/StaticHostProvider.java 1397800
> /src/java/test/org/apache/zookeeper/test/StaticHostProviderTest.java 1397800
>
> Diff: https://reviews.apache.org/r/3781/diff/
>
>
> Testing
> -------
>
> new tests included as part of the patch
>
>
> Thanks,
>
> Alexander Shraer
>
>
Re: Review Request: Add zk.updateServerList(newServerList)
Posted by Alexander Shraer <sh...@yahoo-inc.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3781/
-----------------------------------------------------------
(Updated Nov. 14, 2012, 6:54 a.m.)
Review request for zookeeper.
Changes
-------
this corresponds to ZOOKEEPER-1355-13-Nov-ver2.patch
Description
-------
https://issues.apache.org/jira/browse/ZOOKEEPER-1355
Diffs (updated)
-----
/src/c/Makefile.am 1409078
/src/c/include/zookeeper.h 1409078
/src/c/src/addrvec.h PRE-CREATION
/src/c/src/addrvec.c PRE-CREATION
/src/c/src/mt_adaptor.c 1409078
/src/c/src/st_adaptor.c 1409078
/src/c/src/zk_adaptor.h 1409078
/src/c/src/zookeeper.c 1409078
/src/c/tests/TestReconfig.cc PRE-CREATION
/src/c/tests/TestZookeeperClose.cc 1409078
/src/c/tests/TestZookeeperInit.cc 1409078
/src/c/tests/ZKMocks.cc 1409078
/src/docs/src/documentation/content/xdocs/zookeeperProgrammers.xml 1409078
/src/java/main/org/apache/zookeeper/ZooKeeper.java 1409078
/src/java/main/org/apache/zookeeper/client/HostProvider.java 1409078
/src/java/main/org/apache/zookeeper/client/StaticHostProvider.java 1409078
/src/java/test/org/apache/zookeeper/test/StaticHostProviderTest.java 1409078
Diff: https://reviews.apache.org/r/3781/diff/
Testing
-------
new tests included as part of the patch
Thanks,
Alexander Shraer
Re: Review Request: Add zk.updateServerList(newServerList)
Posted by Alexander Shraer <sh...@yahoo-inc.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3781/
-----------------------------------------------------------
(Updated Nov. 14, 2012, 6:08 a.m.)
Review request for zookeeper.
Changes
-------
Updated diff that addresses all in previous diff, corresponds to ZOOKEEPER-1355-13-Nov.patch
Description
-------
https://issues.apache.org/jira/browse/ZOOKEEPER-1355
Diffs (updated)
-----
/src/c/Makefile.am 1409078
/src/c/include/zookeeper.h 1409078
/src/c/src/addrvec.h PRE-CREATION
/src/c/src/addrvec.c PRE-CREATION
/src/c/src/mt_adaptor.c 1409078
/src/c/src/st_adaptor.c 1409078
/src/c/src/zk_adaptor.h 1409078
/src/c/src/zookeeper.c 1409078
/src/c/tests/TestReconfig.cc PRE-CREATION
/src/c/tests/TestZookeeperClose.cc 1409078
/src/c/tests/TestZookeeperInit.cc 1409078
/src/c/tests/ZKMocks.cc 1409078
/src/docs/src/documentation/content/xdocs/zookeeperProgrammers.xml 1409078
/src/java/main/org/apache/zookeeper/ZooKeeper.java 1409078
/src/java/main/org/apache/zookeeper/client/HostProvider.java 1409078
/src/java/main/org/apache/zookeeper/client/StaticHostProvider.java 1409078
/src/java/test/org/apache/zookeeper/test/StaticHostProviderTest.java 1409078
Diff: https://reviews.apache.org/r/3781/diff/
Testing
-------
new tests included as part of the patch
Thanks,
Alexander Shraer
Re: Review Request: Add zk.updateServerList(newServerList)
Posted by Marshall McMullen <ma...@gmail.com>.
> 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
>
>
Re: Review Request: Add zk.updateServerList(newServerList)
Posted by fp...@apache.org.
-----------------------------------------------------------
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
>
>
Re: Review Request: Add zk.updateServerList(newServerList)
Posted by Alexander Shraer <sh...@yahoo-inc.com>.
-----------------------------------------------------------
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.
Changes
-------
New patch includes Marshall's changes to the C client, addressing Michi's comments
Description
-------
https://issues.apache.org/jira/browse/ZOOKEEPER-1355
Diffs (updated)
-----
/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
Re: Review Request: Add zk.updateServerList(newServerList)
Posted by Alexander Shraer <sh...@yahoo-inc.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3781/
-----------------------------------------------------------
(Updated Oct. 13, 2012, 7:56 a.m.)
Review request for zookeeper.
Changes
-------
Updating to latest patch (ZOOKEEPER-1355-13-Oct.patch). This patch passes all test (see ZK-1355 jira)
Description
-------
https://issues.apache.org/jira/browse/ZOOKEEPER-1355
Diffs (updated)
-----
/src/c/Makefile.am 1397800
/src/c/include/zookeeper.h 1397800
/src/c/src/mt_adaptor.c 1397800
/src/c/src/st_adaptor.c 1397800
/src/c/src/zk_adaptor.h 1397800
/src/c/src/zookeeper.c 1397800
/src/c/tests/TestZookeeperClose.cc 1397800
/src/c/tests/TestZookeeperInit.cc 1397800
/src/c/tests/ZKMocks.cc 1397800
/src/docs/src/documentation/content/xdocs/zookeeperProgrammers.xml 1397800
/src/java/main/org/apache/zookeeper/ZooKeeper.java 1397800
/src/java/main/org/apache/zookeeper/client/HostProvider.java 1397800
/src/java/main/org/apache/zookeeper/client/StaticHostProvider.java 1397800
/src/java/test/org/apache/zookeeper/test/StaticHostProviderTest.java 1397800
Diff: https://reviews.apache.org/r/3781/diff/
Testing
-------
new tests included as part of the patch
Thanks,
Alexander Shraer