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