> On Jan. 27, 2014, 2:43 p.m., Ivan Kelly wrote: > > 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.
I don't know if you really understand what the patch is doing or if you read the comments in the jira. it is fixing the fundamental root cause that we shutdown client in wrong sequence, which cause the problematic in ledger handle and bookkeeper. All the operations in ledger handle and bookkeeper are either metadata operations, or data operations, or the callback chained with metadata/data operations. if we keep tracked the state of ledger manager and bookie client (PCBC) and fixing the wrong closing sequence issue, why it is a wrong direction? For catching rejected exceptions, I think they are inherited from previous patches (which is indeed dealing with the issue in wrong direction). I think they are good to be kept there, so we would have logs that could help us debugging if we are not dealing with the states correctly. As current design, the states in pcbc are channel states, not client state. if a channel is in closed state, it just means the channel is broken and not connected, it doesn't mean that the client is closed. the flag that I added is used for the client state which we could reject any incoming request if the client is to be closed permanently (by calling #close). distinguishing a channel state from client state is good if we want to move support multiple channels to a bookie. >>> I think if we cleanup BookieClient & PCBC as stated above and use the >>> CleanupLedgerManager, we should be able to stop catching the >>> runtimeexceptions. again, if you read the patch correctly, isn't the patch to make BookieClient/PCBC handling close correctly and use CleanupLedgerManager? so I don't know what is you real point? - Sijie ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17352/#review32830 ----------------------------------------------------------- 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 > >
