----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36296/#review90981 -----------------------------------------------------------
Looks good! I just have a few points below, Sijie. bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeper.java (line 239) <https://reviews.apache.org/r/36296/#comment144145> This is changing the semantics of the call a bit. Before we would throw NPE (kind of ugly) in the case zk is null, but now we have decided to create an instance. I think the original intent was to have the application passing a ZK instance and in the case it is null, we assume that the application made a mistake and don't proceed. bookkeeper-server/src/main/java/org/apache/bookkeeper/replication/AutoRecoveryMain.java (line 85) <https://reviews.apache.org/r/36296/#comment144149> In the case of leaving a TODO in the code, it is better to create a jira and tag the TODO with the jira number. bookkeeper-server/src/main/java/org/apache/bookkeeper/zookeeper/ZooKeeperClient.java (line 231) <https://reviews.apache.org/r/36296/#comment144153> This comment is a bit funny :-) bookkeeper-server/src/main/java/org/apache/bookkeeper/zookeeper/ZooKeeperClient.java (line 338) <https://reviews.apache.org/r/36296/#comment144154> I'm not sure why you've made this catch generic (catch Exception), but I was wondering if we should be collapsing the two catches into one. bookkeeper-server/src/main/java/org/apache/bookkeeper/zookeeper/ZooKeeperClient.java (line 434) <https://reviews.apache.org/r/36296/#comment144155> When was this deprecated? If this is coming from branch 4.3, then we should be removing it in 4.4, no? bookkeeper-server/src/test/java/org/apache/bookkeeper/client/BookKeeperClientZKSessionExpiry.java (line 38) <https://reviews.apache.org/r/36296/#comment144157> Did you mean to put down 60000? - fpj On July 8, 2015, 7:31 a.m., Sijie Guo wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/36296/ > ----------------------------------------------------------- > > (Updated July 8, 2015, 7:31 a.m.) > > > Review request for bookkeeper and fpj. > > > Bugs: BOOKKEEPER-796 > https://issues.apache.org/jira/browse/BOOKKEEPER-796 > > > Repository: bookkeeper-git > > > Description > ------- > > change ZooKeeperClient to use builder and add stats support > change zookeeper in bookkeeper to use ZooKeeperClient. > > > Diffs > ----- > > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Bookie.java > 3078ff1 > > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java > 48a76fb > > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/FileSystemUpgrade.java > f6ec59c > > bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeper.java > 49d8e59 > > bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeperAdmin.java > fbca7d2 > > bookkeeper-server/src/main/java/org/apache/bookkeeper/replication/Auditor.java > 4e6e3fb > > bookkeeper-server/src/main/java/org/apache/bookkeeper/replication/AutoRecoveryMain.java > dd08f71 > > bookkeeper-server/src/main/java/org/apache/bookkeeper/util/LocalBookKeeper.java > 0937bb5 > bookkeeper-server/src/main/java/org/apache/bookkeeper/util/MathUtils.java > 4cff2bb > bookkeeper-server/src/main/java/org/apache/bookkeeper/util/ZkUtils.java > 9608240 > > bookkeeper-server/src/main/java/org/apache/bookkeeper/zookeeper/ExponentialBackoffRetryPolicy.java > dab7aa7 > > bookkeeper-server/src/main/java/org/apache/bookkeeper/zookeeper/ZooKeeperClient.java > a479a36 > > bookkeeper-server/src/main/java/org/apache/bookkeeper/zookeeper/ZooKeeperWatcherBase.java > 707b842 > > bookkeeper-server/src/main/java/org/apache/bookkeeper/zookeeper/ZooWorker.java > 871612e > > bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/BookieInitializationTest.java > 8da9eb4 > > bookkeeper-server/src/test/java/org/apache/bookkeeper/client/BookKeeperClientZKSessionExpiry.java > PRE-CREATION > > bookkeeper-server/src/test/java/org/apache/bookkeeper/meta/TestLedgerManager.java > 8fd40c9 > > bookkeeper-server/src/test/java/org/apache/bookkeeper/replication/AuditorBookieTest.java > b8020c6 > > bookkeeper-server/src/test/java/org/apache/bookkeeper/replication/AuditorPeriodicBookieCheckTest.java > 91aae77 > > bookkeeper-server/src/test/java/org/apache/bookkeeper/replication/AuditorPeriodicCheckTest.java > a44eff3 > > bookkeeper-server/src/test/java/org/apache/bookkeeper/replication/TestLedgerUnderreplicationManager.java > ff1bb0f > > bookkeeper-server/src/test/java/org/apache/bookkeeper/replication/TestReplicationWorker.java > d47e4b1 > > bookkeeper-server/src/test/java/org/apache/bookkeeper/test/ZooKeeperUtil.java > 769abbc > > bookkeeper-server/src/test/java/org/apache/bookkeeper/zookeeper/TestZooKeeperClient.java > f48fe37 > > Diff: https://reviews.apache.org/r/36296/diff/ > > > Testing > ------- > > > Thanks, > > Sijie Guo > >
