> On July 8, 2015, 8:34 p.m., fpj wrote: > > bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeper.java, > > line 288 > > <https://reviews.apache.org/r/36296/diff/1/?file=1002030#file1002030line288> > > > > 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.
I think this private constructor is merging two behavior together. since this construtor won't be called by application, we could add the checking in the public constructor to keep the same semantic. does it work for you? > On July 8, 2015, 8:34 p.m., fpj wrote: > > bookkeeper-server/src/main/java/org/apache/bookkeeper/zookeeper/ZooKeeperClient.java, > > line 367 > > <https://reviews.apache.org/r/36296/diff/1/?file=1002038#file1002038line367> > > > > 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. I think the catch is to catch other runtime exceptions. keep those two catches is to keep different logging message. > On July 8, 2015, 8:34 p.m., fpj wrote: > > bookkeeper-server/src/main/java/org/apache/bookkeeper/zookeeper/ZooKeeperClient.java, > > line 465 > > <https://reviews.apache.org/r/36296/diff/1/?file=1002038#file1002038line465> > > > > When was this deprecated? If this is coming from branch 4.3, then we > > should be removing it in 4.4, no? good catch. I think the @Deprecated should be removed - Sijie ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36296/#review90981 ----------------------------------------------------------- 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 > >
