> On 2012-02-08 16:52:38, fpj wrote: > > /src/java/test/org/apache/zookeeper/test/StaticHostProviderTest.java, line > > 142 > > <https://reviews.apache.org/r/3781/diff/1/?file=72955#file72955line142> > > > > 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.
Fixed. StaticHostProvider now gets a seed so we always get the same results in tests > On 2012-02-08 16:52:38, fpj wrote: > > /src/java/test/org/apache/zookeeper/server/quorum/Zab1_0Test.java, line 213 > > <https://reviews.apache.org/r/3781/diff/1/?file=72954#file72954line213> > > > > 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 the problem described in https://issues.apache.org/jira/browse/ZOOKEEPER-1357 if I remember correctly you wanted to take a closer look on this jira - I don't mind whether we solve it as part of this one or separately. > On 2012-02-08 16:52:38, fpj wrote: > > /src/java/test/org/apache/zookeeper/test/StaticHostProviderTest.java, line > > 143 > > <https://reviews.apache.org/r/3781/diff/1/?file=72955#file72955line143> > > > > You may want to have the comments on top of the line, so that it > > doesn't overflow. Fixed > On 2012-02-08 16:52:38, fpj wrote: > > /src/java/main/org/apache/zookeeper/client/StaticHostProvider.java, line 122 > > <https://reviews.apache.org/r/3781/diff/1/?file=72953#file72953line122> > > > > You may want to use {@link ...}. Fixed > On 2012-02-08 16:52:38, fpj wrote: > > /src/java/main/org/apache/zookeeper/client/StaticHostProvider.java, line 84 > > <https://reviews.apache.org/r/3781/diff/1/?file=72953#file72953line84> > > > > It is not clear to me why we need this synchronization block. This is a > > constructor so the object is not available yet. Fixed > On 2012-02-08 16:52:38, fpj wrote: > > /src/java/main/org/apache/zookeeper/ZooKeeper.java, line 109 > > <https://reviews.apache.org/r/3781/diff/1/?file=72951#file72951line109> > > > > You may want to use {@link ...} here. fixed > On 2012-02-08 16:52:38, fpj wrote: > > /src/java/main/org/apache/zookeeper/client/StaticHostProvider.java, line 51 > > <https://reviews.apache.org/r/3781/diff/1/?file=72953#file72953line51> > > > > reconfiguration at this point consists of changing the list of servers > > through the ZooKeeper object, yes? yes - Alexander ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3781/#review4896 ----------------------------------------------------------- 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 > >
