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