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:shra...@yahoo-inc.com] > Sent: Wednesday, February 08, 2012 3:50 PM > To: dev@zookeeper.apache.org; 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: f...@apache.org [mailto:f...@apache.org] > > Sent: Wednesday, February 08, 2012 8:53 AM > > To: f...@apache.org; 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 > > 1236340 > > > /src/java/main/org/apache/zookeeper/ZooKeeper.java 1236340 > > > /src/java/main/org/apache/zookeeper/client/HostProvider.java > > 1236340 > > > > /src/java/main/org/apache/zookeeper/client/StaticHostProvider.java > > 1236340 > > > > /src/docs/src/documentation/content/xdocs/zookeeperProgrammers.xml > > 1236340 > > > > > > Diff: https://reviews.apache.org/r/3781/diff > > > > > > > > > Testing > > > ------- > > > > > > new tests included as part of the patch > > > > > > > > > Thanks, > > > > > > Alexander > > > > > >
