> 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. > > Sijie Guo wrote: > 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?
add the validation back in public constructors. > On July 8, 2015, 8:34 p.m., fpj wrote: > > bookkeeper-server/src/main/java/org/apache/bookkeeper/replication/AutoRecoveryMain.java, > > line 85 > > <https://reviews.apache.org/r/36296/diff/1/?file=1002033#file1002033line85> > > > > 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. added the jira ticket > On July 8, 2015, 8:34 p.m., fpj wrote: > > bookkeeper-server/src/main/java/org/apache/bookkeeper/zookeeper/ZooKeeperClient.java, > > line 255 > > <https://reviews.apache.org/r/36296/diff/1/?file=1002038#file1002038line255> > > > > This comment is a bit funny :-) removed the comment > 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? > > Sijie Guo wrote: > good catch. I think the @Deprecated should be removed removed > On July 8, 2015, 8:34 p.m., fpj wrote: > > bookkeeper-server/src/test/java/org/apache/bookkeeper/client/BookKeeperClientZKSessionExpiry.java, > > line 38 > > <https://reviews.apache.org/r/36296/diff/1/?file=1002042#file1002042line38> > > > > Did you mean to put down 60000? changed to 60000 - 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 > >
