> On Dec. 18, 2014, 5:54 a.m., Sijie Guo wrote: > > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java, > > line 1190 > > <https://reviews.apache.org/r/28835/diff/2/?file=792807#file792807line1190> > > > > final
done > On Dec. 18, 2014, 5:54 a.m., Sijie Guo wrote: > > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java, > > line 1194 > > <https://reviews.apache.org/r/28835/diff/2/?file=792807#file792807line1194> > > > > The bookieId option is really weird. A much clear option would specify > > new bookieId, rather use 'ip' or 'hostname'. > > > > -1 on the option Followed same pattern of cookie updates. Make 'hostname' and 'ip' as constants, so the user could only specify "updatebookie -bookieId hostname" or "updatebookie -bookieId ip". Does this makes sense to you? > On Dec. 18, 2014, 5:54 a.m., Sijie Guo wrote: > > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java, > > line 1214 > > <https://reviews.apache.org/r/28835/diff/2/?file=792807#file792807line1214> > > > > please update this reflecting to new options. ok > On Dec. 18, 2014, 5:54 a.m., Sijie Guo wrote: > > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java, > > line 1226 > > <https://reviews.apache.org/r/28835/diff/2/?file=792807#file792807line1226> > > > > use '{}' ok > On Dec. 18, 2014, 5:54 a.m., Sijie Guo wrote: > > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java, > > line 1238 > > <https://reviews.apache.org/r/28835/diff/2/?file=792807#file792807line1238> > > > > inconsistent parameter ok > On Dec. 18, 2014, 5:54 a.m., Sijie Guo wrote: > > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java, > > line 1240 > > <https://reviews.apache.org/r/28835/diff/2/?file=792807#file792807line1240> > > > > update log statement to reflect correct parameter. > > > > "+" -> "," ok > On Dec. 18, 2014, 5:54 a.m., Sijie Guo wrote: > > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java, > > line 1691 > > <https://reviews.apache.org/r/28835/diff/2/?file=792807#file792807line1691> > > > > static? ok > On Dec. 18, 2014, 5:54 a.m., Sijie Guo wrote: > > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java, > > line 1283 > > <https://reviews.apache.org/r/28835/diff/2/?file=792807#file792807line1283> > > > > trailing spaces ok > On Dec. 18, 2014, 5:54 a.m., Sijie Guo wrote: > > bookkeeper-server/src/main/java/org/apache/bookkeeper/client/UpdateLedgerOp.java, > > line 94 > > <https://reviews.apache.org/r/28835/diff/2/?file=792808#file792808line94> > > > > move run method to a separate method Extract #run logic as #updateLedgers > On Dec. 18, 2014, 5:54 a.m., Sijie Guo wrote: > > bookkeeper-server/src/main/java/org/apache/bookkeeper/client/UpdateLedgerOp.java, > > line 95 > > <https://reviews.apache.org/r/28835/diff/2/?file=792808#file792808line95> > > > > ThreadFactoryBuilder ok > On Dec. 18, 2014, 5:54 a.m., Sijie Guo wrote: > > bookkeeper-server/src/main/java/org/apache/bookkeeper/client/UpdateLedgerOp.java, > > line 105 > > <https://reviews.apache.org/r/28835/diff/2/?file=792808#file792808line105> > > > > outstandings ok > On Dec. 18, 2014, 5:54 a.m., Sijie Guo wrote: > > bookkeeper-server/src/main/java/org/apache/bookkeeper/client/UpdateLedgerOp.java, > > line 118 > > <https://reviews.apache.org/r/28835/diff/2/?file=792808#file792808line118> > > > > why you need #get() here? isn't Runnable called only when readCb is > > completed? readCb.get() is used to see any errors during the ledger update. In case of errors will stop issuing of ledgers and will exit. > On Dec. 18, 2014, 5:54 a.m., Sijie Guo wrote: > > bookkeeper-server/src/main/java/org/apache/bookkeeper/client/UpdateLedgerOp.java, > > line 154 > > <https://reviews.apache.org/r/28835/diff/2/?file=792808#file792808line154> > > > > why #shutdownNow? not shutdown? ok > On Dec. 18, 2014, 5:54 a.m., Sijie Guo wrote: > > bookkeeper-server/src/main/java/org/apache/bookkeeper/client/UpdateLedgerOp.java, > > line 165 > > <https://reviews.apache.org/r/28835/diff/2/?file=792808#file792808line165> > > > > in finally block ok > On Dec. 18, 2014, 5:54 a.m., Sijie Guo wrote: > > bookkeeper-server/src/main/java/org/apache/bookkeeper/client/UpdateLedgerOp.java, > > line 183 > > <https://reviews.apache.org/r/28835/diff/2/?file=792808#file792808line183> > > > > 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(...) > > } > > > > } Its new learning for me. I tried using the SettableFuture in latest patch. Please let me know the usage is proper. thanks! I met the following exception as the listener is a kind of async notification. What I'm thinking is, readCb.get() will comeout immedaitely after future.set() is called but before invoking the registered listener. Now during the execution there could be chances of finshing the main thread and pExecutor shutdown. I feel the below exception is expected and is OK. 2014-12-21 22:54:07,268 - INFO - [UpdateLedgerThread:UpdateLedgerOp$1@142] - Total number of ledgers updated = 1 Dec 21, 2014 10:54:07 PM com.google.common.util.concurrent.ExecutionList$RunnableExecutorPair execute SEVERE: RuntimeException while executing runnable org.apache.bookkeeper.client.UpdateLedgerOp$1$1@7da1f182 with executor java.util.concurrent.Executors$FinalizableDelegatedExecutorService@34741c9d java.util.concurrent.RejectedExecutionException: Task org.apache.bookkeeper.client.UpdateLedgerOp$1$1@7da1f182 rejected from java.util.concurrent.ThreadPoolExecutor@f64cb82[Terminated, pool size = 0, active threads = 0, queued tasks = 0, completed tasks = 0] at java.util.concurrent.ThreadPoolExecutor$AbortPolicy.rejectedExecution(Unknown Source) at java.util.concurrent.ThreadPoolExecutor.reject(Unknown Source) at java.util.concurrent.ThreadPoolExecutor.execute(Unknown Source) at java.util.concurrent.Executors$DelegatedExecutorService.execute(Unknown Source) at com.google.common.util.concurrent.ExecutionList$RunnableExecutorPair.execute(ExecutionList.java:149) at com.google.common.util.concurrent.ExecutionList.execute(ExecutionList.java:134) at com.google.common.util.concurrent.AbstractFuture.set(AbstractFuture.java:175) at com.google.common.util.concurrent.SettableFuture.set(SettableFuture.java:53) at org.apache.bookkeeper.client.UpdateLedgerOp$ReadLedgerMetadataCb$1.operationComplete(UpdateLedgerOp.java:241) at org.apache.bookkeeper.client.UpdateLedgerOp$ReadLedgerMetadataCb$1.operationComplete(UpdateLedgerOp.java:1) at org.apache.bookkeeper.meta.CleanupLedgerManager$CleanupGenericCallback.operationComplete(CleanupLedgerManager.java:51) at org.apache.bookkeeper.meta.AbstractZkLedgerManager$3.processResult(AbstractZkLedgerManager.java:360) at org.apache.zookeeper.ClientCnxn$EventThread.processEvent(ClientCnxn.java:545) at org.apache.zookeeper.ClientCnxn$EventThread.run(ClientCnxn.java:497) > On Dec. 18, 2014, 5:54 a.m., Sijie Guo wrote: > > bookkeeper-server/src/main/java/org/apache/bookkeeper/client/UpdateLedgerOp.java, > > line 194 > > <https://reviews.apache.org/r/28835/diff/2/?file=792808#file792808line194> > > > > please remove newObject[]. you don't need this ok > On Dec. 18, 2014, 5:54 a.m., Sijie Guo wrote: > > bookkeeper-server/src/main/java/org/apache/bookkeeper/client/UpdateLedgerOp.java, > > line 215 > > <https://reviews.apache.org/r/28835/diff/2/?file=792808#file792808line215> > > > > remove new Object[] {} ok > On Dec. 18, 2014, 5:54 a.m., Sijie Guo wrote: > > bookkeeper-server/src/test/java/org/apache/bookkeeper/client/UpdateLedgerOpTest.java, > > line 71 > > <https://reviews.apache.org/r/28835/diff/2/?file=792809#file792809line71> > > > > why you need add any entries? it is just a metadata change. it'd better > > to not add any entries. ok > On Dec. 18, 2014, 5:54 a.m., Sijie Guo wrote: > > bookkeeper-server/src/test/java/org/apache/bookkeeper/client/UpdateLedgerOpTest.java, > > line 91 > > <https://reviews.apache.org/r/28835/diff/2/?file=792809#file792809line91> > > > > please add Assert.assertFalse(ensemble.contains(curBookieAddr)) ok > On Dec. 18, 2014, 5:54 a.m., Sijie Guo wrote: > > bookkeeper-server/src/test/java/org/apache/bookkeeper/client/UpdateLedgerOpTest.java, > > line 109 > > <https://reviews.apache.org/r/28835/diff/2/?file=792809#file792809line109> > > > > change entries to 0 ok - Rakesh ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28835/#review65458 ----------------------------------------------------------- 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 > >
