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

Switch to Plain View
Zookeeper >> mail # dev >> Re: Review Request: Add zk.updateServerList(newServerList)


+
Marshall McMullen 2012-10-16, 15:11
+
Marshall McMullen 2012-11-06, 16:05
+
Marshall McMullen 2012-11-06, 16:14
+
Alexander Shraer 2012-11-06, 23:40
+
fpj@... 2012-11-13, 17:02
Copy link to this message
-
Re: Review Request: Add zk.updateServerList(newServerList)


> 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:
+
Alexander Shraer 2012-11-14, 06:08
+
Alexander Shraer 2012-11-14, 06:54