> On Dec. 9, 2014, 12:08 p.m., Ivan Kelly wrote: > > bookkeeper-server/src/main/java/org/apache/bookkeeper/admin/UpdateLedgerOp.java, > > line 82 > > <https://reviews.apache.org/r/28835/diff/1/?file=786287#file786287line82> > > > > Latch is not needed. The executor will return a future. just call > > .get() on it to get the same behaviour as the latch. > > Rakesh R wrote: > yes. I will do it. > > Rakesh R wrote: > I feel latch is simple to control the outerloop. What do you say? > > Ivan Kelly wrote: > but it's unnecessary. submit() returns a future. you just need to call > .get() on that to have the exact same effect. > > Rakesh R wrote: > oops, I missed it. Will do:) > > Rakesh R wrote: > Hi Ivan, I'm reopening this comment. In the latest patch I've used latch > to control syncing up. > > I'm using FutureCallback to update the updatedLedgerCount. Now the > updation of each ledger will be completed only after the futurecallback > execution. When I tested I've observed that #get() api is coming out before > invoking its futureCallback, this may give wrong log messages. For accuracy > all outstanding ledgers needs to wait till the callback completion. > > Do you agree to use latch way of controlling the sequence? > > Ivan Kelly wrote: > I don't understand the comment. Do you mean that submit(new > Runnable(){...}).get() is returning before all callbacks have been completed? > If this is the case, then latch doesn't solve it. The latch, as it is now, is > functionally equivalent to calling get(). Timing may be different though. If > it doesn't run correctly using the get() then there's probably a race > condition lurking somewhere.
It seems my explanation was bad:( >>>>>>>>>>The latch, as it is now, is functionally equivalent to calling get() Latch is used to see all the outstandings future#callback() execution is completed rather than outstandings future#get() Let me try to add more explanation. In the latest patch, it will register callback to the futures and this callback will be run when the Future's computation is complete. Callback has the logic to update the status of each ledger updation(success or failure). Algorithm: ---------- 1) Set syncObj; // outer <Begin: iterate and for each ledger do> 1.1) Acquire thrttle 1.2)Creates ReadLedgerMetadataCb 1.3) Adds the respective ledgerid to 'outstandings' 1.4) Register Callback to update the status(success or failure) of the future. Also, callback has the logic to notify 'syncObj' 1.5) Update issuedLedgerCnt 1.6) Check & set the stop flag=true, if limit reaches 1.7) Call updateLedger operation <End:> 2) syncObj.await() -> syncObj will be notified only after all the issued ledgers finishes executing the registered callback. On callback, it will remove the respective ledgerid from 'outstandings' and updatedLedgerCnt#incr(). Finally, Callback will check whether outstandings#isEmpty() && stop==true, if yes notifies syncObj. Initially I tried using ReadLedgerMetadataCb#get() to see there is no outstandings. But I've noticied, this call comes out before the callback execution finishes and is giving wrong 'updatedLedgerCnt' value. I meant, return of ReadLedgerMetadataCb#get() is not guarantees that its registered callback is triggered and executed. Thats the reason I tried using separate 'latch' to mark the completion of all the issued ledgers. Does this makes sense to you? - Rakesh ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28835/#review64360 ----------------------------------------------------------- On Jan. 1, 2015, 7:43 p.m., Rakesh R wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/28835/ > ----------------------------------------------------------- > > (Updated Jan. 1, 2015, 7:43 p.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/UpdateLedgerCmdTest.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 > >
