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)
Hi Flavio,

Thanks, and I agree with your comments. Regarding the last one I'll see if we can pass a Random object to StaticHostProvider so that we can initialize it with the same seed when doing the tests. I would prefer that over executing multiple times.

Alex

> -----Original Message-----
> From: [EMAIL PROTECTED] [mailto:[EMAIL PROTECTED]]
> Sent: Wednesday, February 08, 2012 8:53 AM
> To: [EMAIL PROTECTED]; zookeeper; Alexander Shraer
> Subject: Re: Review Request: Add zk.updateServerList(newServerList)
>
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3781/#review4896
> -----------------------------------------------------------
>
>
> It looks very good, Alex. I have just a few comments below.
>
> I was also wondering about the use cases for this. Zookeeper clients
> could use it directly, but it is not clear if it is they main use case.
> It might be a good idea to clarify somewhere: documentation, wiki, or
> jira.
>
> It sounds like there are a couple of related jiras that would be good
> to link to this one if they are really related:
>
> - Re-resolving DNS hosnames (ZOOKEEPER-338?)
> - Specifying user lists with a URL (ZOOKEEPER-390?)
>
> I'm assuming that those will use the functionality of this patch. Is
> this correct?
>
>
> /src/java/main/org/apache/zookeeper/ZooKeeper.java
> <https://reviews.apache.org/r/3781/#comment10802>
>
>     You may want to use {@link ...} here.
>
>
>
> /src/java/main/org/apache/zookeeper/client/StaticHostProvider.java
> <https://reviews.apache.org/r/3781/#comment10797>
>
>     reconfiguration at this point consists of changing the list of
> servers through the ZooKeeper object, yes?
>
>
>
> /src/java/main/org/apache/zookeeper/client/StaticHostProvider.java
> <https://reviews.apache.org/r/3781/#comment10794>
>
>     It is not clear to me why we need this synchronization block. This
> is a constructor so the object is not available yet.
>
>
>
> /src/java/main/org/apache/zookeeper/client/StaticHostProvider.java
> <https://reviews.apache.org/r/3781/#comment10803>
>
>     You may want to use {@link ...}.
>
>
>
> /src/java/test/org/apache/zookeeper/server/quorum/Zab1_0Test.java
> <https://reviews.apache.org/r/3781/#comment10800>
>
>     This change seems to be gratuitous with respect to this new
> feature. It is ok, though, but I was trying to understand why it is
> here.
>
>
>
> /src/java/test/org/apache/zookeeper/test/StaticHostProviderTest.java
> <https://reviews.apache.org/r/3781/#comment10801>
>
>     If I understand correctly, this assertion will probably fail
> sometimes. I understand that the approach is probabilistic and it is
> not possible to predict the outcome. But, we could test that it at
> least provides both outcomes: disconnect and don't disconnect.
> Something like that would prevent some false positives when running
> tests.
>
>     If you want to test that you're getting the correct ratio of
> disconnects to no-disconnects without false positives, then one way
> might be to use a fixed seed so that you always get the same order.
>
>
>
> /src/java/test/org/apache/zookeeper/test/StaticHostProviderTest.java
> <https://reviews.apache.org/r/3781/#comment10804>
>
>     You may want to have the comments on top of the line, so that it
> doesn't overflow.
>
>
> - fpj
>
>
> On 2012-02-07 22:42:04, Alexander Shraer wrote:
> >
> > -----------------------------------------------------------
> > This is an automatically generated e-mail. To reply, visit:
> > https://reviews.apache.org/r/3781/
> > -----------------------------------------------------------
> >
> > (Updated 2012-02-07 22:42:04)
> >
> >
> > Review request for zookeeper.
> >
> >
> > Summary
> > -------
> >
> > https://issues.apache.org/jira/browse/ZOOKEEPER-1355
> >
> >
> > Diffs
> > -----
> >
> >   /src/java/test/org/apache/zookeeper/server/quorum/Zab1_0Test.java
> 1236340
> >
> /src/java/test/org/apache/zookeeper/test/StaticHostProviderTest.java