Home | About | Sematext search-lucene.com search-hadoop.com
 Search Hadoop and all its subprojects:

Switch to Threaded View
Zookeeper, mail # dev - Re: Review Request: Add zk.updateServerList(newServerList)


Copy link to this message
-
Re: Review Request: Add zk.updateServerList(newServerList)
fpj@... 2012-11-13, 17:02

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