----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28835/#review65458 -----------------------------------------------------------
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java <https://reviews.apache.org/r/28835/#comment108606> final bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java <https://reviews.apache.org/r/28835/#comment108611> The bookieId option is really weird. A much clear option would specify new bookieId, rather use 'ip' or 'hostname'. -1 on the option bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java <https://reviews.apache.org/r/28835/#comment108604> trailing space bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java <https://reviews.apache.org/r/28835/#comment108607> please update this reflecting to new options. bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java <https://reviews.apache.org/r/28835/#comment108608> use '{}' bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java <https://reviews.apache.org/r/28835/#comment108609> inconsistent parameter bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java <https://reviews.apache.org/r/28835/#comment108610> update log statement to reflect correct parameter. "+" -> "," bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java <https://reviews.apache.org/r/28835/#comment108612> trailing spaces bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java <https://reviews.apache.org/r/28835/#comment108613> static? bookkeeper-server/src/main/java/org/apache/bookkeeper/client/UpdateLedgerOp.java <https://reviews.apache.org/r/28835/#comment108616> move run method to a separate method bookkeeper-server/src/main/java/org/apache/bookkeeper/client/UpdateLedgerOp.java <https://reviews.apache.org/r/28835/#comment108614> ThreadFactoryBuilder bookkeeper-server/src/main/java/org/apache/bookkeeper/client/UpdateLedgerOp.java <https://reviews.apache.org/r/28835/#comment108619> outstandings bookkeeper-server/src/main/java/org/apache/bookkeeper/client/UpdateLedgerOp.java <https://reviews.apache.org/r/28835/#comment108617> why you need #get() here? isn't Runnable called only when readCb is completed? bookkeeper-server/src/main/java/org/apache/bookkeeper/client/UpdateLedgerOp.java <https://reviews.apache.org/r/28835/#comment108618> why #shutdownNow? not shutdown? bookkeeper-server/src/main/java/org/apache/bookkeeper/client/UpdateLedgerOp.java <https://reviews.apache.org/r/28835/#comment108615> in finally block bookkeeper-server/src/main/java/org/apache/bookkeeper/client/UpdateLedgerOp.java <https://reviews.apache.org/r/28835/#comment108623> this is totally wrong. #addListener is "to Registers a listener to be run on the given executor. The listener will run when the Future's computation is complete or, if the computation is already complete, immediately." in your implementation, you submit the runnable immediately, without checking if the future's compuation is complete or not. you should use SettableFuture<Void> private final static class ReadLedgerMetadataCb .. { final SettableFuture<Void> resultFuture = ... public addListener(...) { resultFuture.addListener(...); } public void operationComplete(...) { resultFuture.set(...) } } bookkeeper-server/src/main/java/org/apache/bookkeeper/client/UpdateLedgerOp.java <https://reviews.apache.org/r/28835/#comment108620> please remove newObject[]. you don't need this bookkeeper-server/src/main/java/org/apache/bookkeeper/client/UpdateLedgerOp.java <https://reviews.apache.org/r/28835/#comment108624> remove new Object[] {} bookkeeper-server/src/test/java/org/apache/bookkeeper/client/UpdateLedgerOpTest.java <https://reviews.apache.org/r/28835/#comment108628> why you need add any entries? it is just a metadata change. it'd better to not add any entries. bookkeeper-server/src/test/java/org/apache/bookkeeper/client/UpdateLedgerOpTest.java <https://reviews.apache.org/r/28835/#comment108625> please add Assert.assertFalse(ensemble.contains(curBookieAddr)) bookkeeper-server/src/test/java/org/apache/bookkeeper/client/UpdateLedgerOpTest.java <https://reviews.apache.org/r/28835/#comment108629> change entries to 0 - Sijie Guo On Dec. 16, 2014, 4:07 a.m., Rakesh R wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/28835/ > ----------------------------------------------------------- > > (Updated Dec. 16, 2014, 4:07 a.m.) > > > Review request for bookkeeper, fpj, Ivan Kelly, and Sijie Guo. > > > Bugs: BOKKEEPER-634 > https://issues.apache.org/jira/browse/BOKKEEPER-634 > > > Repository: bookkeeper-git > > > Description > ------- > > admin tool for changing the bookie's identifier to IP/hostname > > > Diffs > ----- > > > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java > a1e4639 > > bookkeeper-server/src/main/java/org/apache/bookkeeper/client/UpdateLedgerOp.java > PRE-CREATION > > bookkeeper-server/src/test/java/org/apache/bookkeeper/client/UpdateLedgerOpTest.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/28835/diff/ > > > Testing > ------- > > Added few tests > > > Thanks, > > Rakesh R > >
