----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17352/#review32830 -----------------------------------------------------------
I don't like how this patch deals with the fundamental problem, which is that we're not keeping track of our state in our code. PCBC, BookieClient, LedgerHandle, BookKeeper, LedgerManager are all state machines. PCBC is complex, but most have the states (READY, CLOSING, CLOSED). Basically, while in the READY state they should accept new user requests. In the CLOSING state they should start shutting down, and then when all resources have been freed they should move the the CLOSED state. For example, with PCBC, when in the READY state (which is CONNECTED in the code), if it recieves a close request, it should move to the CLOSING state, error out all requests and close the channel, when all these ops are complete it should move to CLOSED. Similarly, in READY state, BookieClient should serve requests. A close request moves it to CLOSING during which it waits for all PCBC to close before moving into CLOSED. In the patch you did something similar for CleanupLedgerManager, except you used a boolean rather than a explicit state field. It's BookKeeper and LedgerHandle which are problematic here, since the state is scattered all over the place. I think if we cleanup BookieClient & PCBC as stated above and use the CleanupLedgerManager, we should be able to stop catching the runtimeexceptions. Actually the BookieClient changes go a lot of the way, but the problem is addEntry which reach into PCBC then reach back out around again to call into PCBC. This is a little rambly, but to summarize, I think the patch deals with the problem from the wrong direction in places. We shouldn't be catching RejectedExectionExceptions because we should be tracking our state enough not to need to. bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookieWatcher.java <https://reviews.apache.org/r/17352/#comment61835> It'd be better to only catch RejectedExecutionException, and only log at debug level since it occurs quite often. bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerHandle.java <https://reviews.apache.org/r/17352/#comment61836> Again, i think rejectedexecutionexception would be better. Another thing we need to be careful about is if errorOutPendingAdds() would try to kick off anything else. For example, if there are recovery adds occurring, where do the callbacks go? I don't have a direct answer, because it's hard to follow now, since internal callbacks and client callbacks are mixed and never actually tracked. bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PerChannelBookieClient.java <https://reviews.apache.org/r/17352/#comment61837> this is just replicating the state variable above. - Ivan Kelly On Jan. 25, 2014, 7:59 a.m., Sijie Guo wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/17352/ > ----------------------------------------------------------- > > (Updated Jan. 25, 2014, 7:59 a.m.) > > > Review request for bookkeeper, fpj and Ivan Kelly. > > > Bugs: BOOKKEEPER-654 > https://issues.apache.org/jira/browse/BOOKKEEPER-654 > > > Repository: bookkeeper-git > > > Description > ------- > > the correct close sequence should be: > > 1) close the bookie client to error out all pending bookie requests, and > after bookie client is close, all following request would be rejected. > 2) close the ledger manager which erred out all pending all metadata > requests, and after ledger manager is close, all metadata request would be > rejected. > 3) close scheduler. > > attach a patch following this sequence. > > > Diffs > ----- > > > bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BKException.java > ace1409 > > bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeper.java > a91861c > > bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeperAdmin.java > c5f5233 > > bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookieWatcher.java > cfb6022 > > bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerChecker.java > cfb9128 > > bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerFragmentReplicator.java > 4a4eb49 > > bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerHandle.java > bf4bd97 > > bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerOpenOp.java > 5b8a703 > > bookkeeper-server/src/main/java/org/apache/bookkeeper/client/PendingAddOp.java > 8f1f18a > > bookkeeper-server/src/main/java/org/apache/bookkeeper/client/PendingReadOp.java > 6cf6c1b > > bookkeeper-server/src/main/java/org/apache/bookkeeper/client/ReadLastConfirmedOp.java > a077556 > > bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/CleanupLedgerManager.java > PRE-CREATION > > bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieClient.java > 696bcc2 > > bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PerChannelBookieClient.java > d8ebaf6 > > bookkeeper-server/src/main/java/org/apache/bookkeeper/util/OrderedSafeExecutor.java > ac068c9 > > bookkeeper-server/src/test/java/org/apache/bookkeeper/client/BookKeeperCloseTest.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/17352/diff/ > > > Testing > ------- > > > Thanks, > > Sijie Guo > >
