> 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. > > Rakesh R wrote: > 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? > > Ivan Kelly wrote: > Actually, the misunderstanding was mine here. When I read your comment > and clicked to look at the source, it brought me to the first patch, rather > than the latest. I was talking about the original latch, which is gone now. > The way you use syncObj is perfectly good. I've only made one more comment > (about throwing exceptions). When that's addressed, this change is ready to > go.
OK. - Rakesh ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28835/#review64360 ----------------------------------------------------------- On Jan. 11, 2015, 3:41 p.m., Rakesh R wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/28835/ > ----------------------------------------------------------- > > (Updated Jan. 11, 2015, 3:41 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 > >
