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)


+
Alexander Shraer 2012-02-08, 23:50
Copy link to this message
-
RE: Review Request: Add zk.updateServerList(newServerList)
Hi Flavio,

I forgot to mention, with regard to one of your comments

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

This solves a problem I mentioned here: https://issues.apache.org/jira/browse/ZOOKEEPER-1343
You mentioned that you'd like to look on the problem more carefully before rushing into a fix, so I opened a Jira for this issue : https://issues.apache.org/jira/browse/ZOOKEEPER-1357

Thanks,
Alex
> -----Original Message-----
> From: Alexander Shraer [mailto:[EMAIL PROTECTED]]
> Sent: Wednesday, February 08, 2012 3:50 PM
> To: [EMAIL PROTECTED]; Flavio Junqueira
> Subject: 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.
+
Marshall McMullen 2012-10-16, 15:11