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, <[EMAIL PROTECTED]> 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
> >
> >
>
>

> 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
>
>

> 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
>
>

-----------------------------------------------------------
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

-----------------------------------------------------------
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

> 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>

Fixed.

Fixed.

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. 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

-----------------------------------------------------------
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