You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@bookkeeper.apache.org by Sijie Guo <gu...@gmail.com> on 2014/01/25 08:59:52 UTC
Review Request 17352: BOOKKEEPER-654: Bookkeeper client operations are
allowed even after its closure, bk#close()
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17352/
-----------------------------------------------------------
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
Re: Review Request 17352: BOOKKEEPER-654: Bookkeeper client operations are
allowed even after its closure, bk#close()
Posted by Sijie Guo <gu...@gmail.com>.
> On Feb. 3, 2014, 10:47 a.m., Ivan Kelly wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/util/OrderedSafeExecutor.java, line 168
> > <https://reviews.apache.org/r/17352/diff/2/?file=462716#file462716line168>
> >
> > This looks very deadlock prone. If a callback is safe to run in the thread context of the caller, why aren't we doing it already? I would suggest just logging the warning every time. If the client hangs, thats a bug in our code.
done
- Sijie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17352/#review33421
-----------------------------------------------------------
On Feb. 4, 2014, 9:07 a.m., Sijie Guo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17352/
> -----------------------------------------------------------
>
> (Updated Feb. 4, 2014, 9:07 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
>
>
Re: Review Request 17352: BOOKKEEPER-654: Bookkeeper client operations are
allowed even after its closure, bk#close()
Posted by Ivan Kelly <iv...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17352/#review33421
-----------------------------------------------------------
bookkeeper-server/src/main/java/org/apache/bookkeeper/util/OrderedSafeExecutor.java
<https://reviews.apache.org/r/17352/#comment62866>
This looks very deadlock prone. If a callback is safe to run in the thread context of the caller, why aren't we doing it already? I would suggest just logging the warning every time. If the client hangs, thats a bug in our code.
- Ivan Kelly
On Feb. 2, 2014, 6:34 a.m., Sijie Guo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17352/
> -----------------------------------------------------------
>
> (Updated Feb. 2, 2014, 6:34 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
>
>
Re: Review Request 17352: BOOKKEEPER-654: Bookkeeper client operations are
allowed even after its closure, bk#close()
Posted by Ivan Kelly <iv...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17352/#review33601
-----------------------------------------------------------
bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PerChannelBookieClient.java
<https://reviews.apache.org/r/17352/#comment63133>
These are still duplicating the state field right above it.
- Ivan Kelly
On Feb. 4, 2014, 9:07 a.m., Sijie Guo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17352/
> -----------------------------------------------------------
>
> (Updated Feb. 4, 2014, 9:07 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
>
>
Re: Review Request 17352: BOOKKEEPER-654: Bookkeeper client operations are
allowed even after its closure, bk#close()
Posted by Sijie Guo <gu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17352/
-----------------------------------------------------------
(Updated Feb. 10, 2014, 6:27 a.m.)
Review request for bookkeeper, fpj and Ivan Kelly.
Changes
-------
addressed Ivan' comment, removed closed flag in pcbc.
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 (updated)
-----
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BKException.java ace1409
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeper.java 441cff6
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeperAdmin.java 7967a82
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookieWatcher.java 744d9c6
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerChecker.java 962f3a3
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerFragmentReplicator.java 7329ba1
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerHandle.java 13d6aa7
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerOpenOp.java 5b8a703
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/PendingAddOp.java f464ac5
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/PendingReadOp.java 241fdbb
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 2f447d9
bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PerChannelBookieClient.java 0757f87
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
Re: Review Request 17352: BOOKKEEPER-654: Bookkeeper client operations are
allowed even after its closure, bk#close()
Posted by Sijie Guo <gu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17352/
-----------------------------------------------------------
(Updated Feb. 4, 2014, 9:07 a.m.)
Review request for bookkeeper, fpj and Ivan Kelly.
Changes
-------
address comment & add close flag in bookkeeper
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 (updated)
-----
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
Re: Review Request 17352: BOOKKEEPER-654: Bookkeeper client operations are
allowed even after its closure, bk#close()
Posted by Sijie Guo <gu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17352/
-----------------------------------------------------------
(Updated Feb. 2, 2014, 6:34 a.m.)
Review request for bookkeeper, fpj and Ivan Kelly.
Changes
-------
change RuntimeException to RejectedExecutionException
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 (updated)
-----
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
Re: Review Request 17352: BOOKKEEPER-654: Bookkeeper client operations are
allowed even after its closure, bk#close()
Posted by Sijie Guo <gu...@gmail.com>.
> On Jan. 27, 2014, 2:43 p.m., Ivan Kelly wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerHandle.java, line 251
> > <https://reviews.apache.org/r/17352/diff/1/?file=450917#file450917line251>
> >
> > 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.
the executor is closed after the ledger manager & bookie client is closed, so any requests after then would be rejected correctly.
- Sijie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17352/#review32830
-----------------------------------------------------------
On Feb. 2, 2014, 6:34 a.m., Sijie Guo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17352/
> -----------------------------------------------------------
>
> (Updated Feb. 2, 2014, 6:34 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
>
>
Re: Review Request 17352: BOOKKEEPER-654: Bookkeeper client operations are
allowed even after its closure, bk#close()
Posted by fp...@apache.org.
> 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.
>
> Sijie Guo wrote:
> 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?
>
> fpj wrote:
> I like the idea of reasoning about this problem using state machines; however, I'm not convinced that considering four separate state machines is the right thing to do. We can think of an overall state machine for the client (e.g., CONNECTING, READY, CLOSED) and state machines per component.
>
> Sijie Guo wrote:
> is there any difference between using a flag than a state for the whole client? or stepping back, do we really need so many state for the whole client?
>
> will the state in ledger manager and the state in bookie client (pcbc) not be enough to define the state of the whole client?
>
> fpj wrote:
> It wouldn't be surprising if we can do with one state machine for the whole client. My thinking is that it might be useful for one global client to translate into multiple states for a component. For example, the state of the overall client might be CLOSED, meaning that the app can't submit new requests, but the state of BookieClient/PCBC might be CLOSING because we are still erroring out pending requests. Making such a distinction might not be strictly necessary, but it doesn't sound wrong to have it.
>
> Sijie Guo wrote:
> the reason that I only did for BookieClient(PCBC) and LedgerManager is for performance consideration. Since BookieClient(PCBC) and LedgerManager already defined the state of BookKeeper (All data operations will be rejected when BookieClient is closing, while all metadata operations will be rejected when LedgerManager is closing), why we need additional layer for it?
>
> Ivan Kelly wrote:
> re: flags vs state, flags are state. Once you go past one flag, you should move to a more explicit state mechanism.
>
> You cannot do this with one state machine for the whole client. Each individual channel has it's own state, so each need their own state machine. In fact, pcbc already is a state machine.
>
> I think that state machines for bookie client and ledgermanager may be enough for now. Changing it for the whole client is a pretty mammoth task.
We need a state machine to represent the states the user sees, like starting, ready, closed. You may claim that this is the state machine of bookkeeper, but I would say that this state machine we expose reflects the state of the overall client. You can surely have a different state machine per component and map the states we expose to applications to states of the different components.
- fpj
-----------------------------------------------------------
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
>
>
Re: Review Request 17352: BOOKKEEPER-654: Bookkeeper client operations are
allowed even after its closure, bk#close()
Posted by Ivan Kelly <iv...@apache.org>.
> 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.
>
> Sijie Guo wrote:
> 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?
>
> fpj wrote:
> I like the idea of reasoning about this problem using state machines; however, I'm not convinced that considering four separate state machines is the right thing to do. We can think of an overall state machine for the client (e.g., CONNECTING, READY, CLOSED) and state machines per component.
>
> Sijie Guo wrote:
> is there any difference between using a flag than a state for the whole client? or stepping back, do we really need so many state for the whole client?
>
> will the state in ledger manager and the state in bookie client (pcbc) not be enough to define the state of the whole client?
>
> fpj wrote:
> It wouldn't be surprising if we can do with one state machine for the whole client. My thinking is that it might be useful for one global client to translate into multiple states for a component. For example, the state of the overall client might be CLOSED, meaning that the app can't submit new requests, but the state of BookieClient/PCBC might be CLOSING because we are still erroring out pending requests. Making such a distinction might not be strictly necessary, but it doesn't sound wrong to have it.
>
> Sijie Guo wrote:
> the reason that I only did for BookieClient(PCBC) and LedgerManager is for performance consideration. Since BookieClient(PCBC) and LedgerManager already defined the state of BookKeeper (All data operations will be rejected when BookieClient is closing, while all metadata operations will be rejected when LedgerManager is closing), why we need additional layer for it?
re: flags vs state, flags are state. Once you go past one flag, you should move to a more explicit state mechanism.
You cannot do this with one state machine for the whole client. Each individual channel has it's own state, so each need their own state machine. In fact, pcbc already is a state machine.
I think that state machines for bookie client and ledgermanager may be enough for now. Changing it for the whole client is a pretty mammoth task.
- Ivan
-----------------------------------------------------------
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
>
>
Re: Review Request 17352: BOOKKEEPER-654: Bookkeeper client operations are
allowed even after its closure, bk#close()
Posted by Sijie Guo <gu...@gmail.com>.
> 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.
>
> Sijie Guo wrote:
> 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?
>
> fpj wrote:
> I like the idea of reasoning about this problem using state machines; however, I'm not convinced that considering four separate state machines is the right thing to do. We can think of an overall state machine for the client (e.g., CONNECTING, READY, CLOSED) and state machines per component.
>
> Sijie Guo wrote:
> is there any difference between using a flag than a state for the whole client? or stepping back, do we really need so many state for the whole client?
>
> will the state in ledger manager and the state in bookie client (pcbc) not be enough to define the state of the whole client?
>
> fpj wrote:
> It wouldn't be surprising if we can do with one state machine for the whole client. My thinking is that it might be useful for one global client to translate into multiple states for a component. For example, the state of the overall client might be CLOSED, meaning that the app can't submit new requests, but the state of BookieClient/PCBC might be CLOSING because we are still erroring out pending requests. Making such a distinction might not be strictly necessary, but it doesn't sound wrong to have it.
the reason that I only did for BookieClient(PCBC) and LedgerManager is for performance consideration. Since BookieClient(PCBC) and LedgerManager already defined the state of BookKeeper (All data operations will be rejected when BookieClient is closing, while all metadata operations will be rejected when LedgerManager is closing), why we need additional layer for it?
- 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
>
>
Re: Review Request 17352: BOOKKEEPER-654: Bookkeeper client operations are
allowed even after its closure, bk#close()
Posted by fp...@apache.org.
> 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.
>
> Sijie Guo wrote:
> 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?
I like the idea of reasoning about this problem using state machines; however, I'm not convinced that considering four separate state machines is the right thing to do. We can think of an overall state machine for the client (e.g., CONNECTING, READY, CLOSED) and state machines per component.
- fpj
-----------------------------------------------------------
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
>
>
Re: Review Request 17352: BOOKKEEPER-654: Bookkeeper client operations are
allowed even after its closure, bk#close()
Posted by Sijie Guo <gu...@gmail.com>.
> 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
>
>
Re: Review Request 17352: BOOKKEEPER-654: Bookkeeper client operations are
allowed even after its closure, bk#close()
Posted by Sijie Guo <gu...@gmail.com>.
> 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.
>
> Sijie Guo wrote:
> 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?
>
> fpj wrote:
> I like the idea of reasoning about this problem using state machines; however, I'm not convinced that considering four separate state machines is the right thing to do. We can think of an overall state machine for the client (e.g., CONNECTING, READY, CLOSED) and state machines per component.
is there any difference between using a flag than a state for the whole client? or stepping back, do we really need so many state for the whole client?
will the state in ledger manager and the state in bookie client (pcbc) not be enough to define the state of the whole client?
- 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
>
>
Re: Review Request 17352: BOOKKEEPER-654: Bookkeeper client operations are
allowed even after its closure, bk#close()
Posted by Sijie Guo <gu...@gmail.com>.
> 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.
>
> Sijie Guo wrote:
> 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?
>
> fpj wrote:
> I like the idea of reasoning about this problem using state machines; however, I'm not convinced that considering four separate state machines is the right thing to do. We can think of an overall state machine for the client (e.g., CONNECTING, READY, CLOSED) and state machines per component.
>
> Sijie Guo wrote:
> is there any difference between using a flag than a state for the whole client? or stepping back, do we really need so many state for the whole client?
>
> will the state in ledger manager and the state in bookie client (pcbc) not be enough to define the state of the whole client?
>
> fpj wrote:
> It wouldn't be surprising if we can do with one state machine for the whole client. My thinking is that it might be useful for one global client to translate into multiple states for a component. For example, the state of the overall client might be CLOSED, meaning that the app can't submit new requests, but the state of BookieClient/PCBC might be CLOSING because we are still erroring out pending requests. Making such a distinction might not be strictly necessary, but it doesn't sound wrong to have it.
>
> Sijie Guo wrote:
> the reason that I only did for BookieClient(PCBC) and LedgerManager is for performance consideration. Since BookieClient(PCBC) and LedgerManager already defined the state of BookKeeper (All data operations will be rejected when BookieClient is closing, while all metadata operations will be rejected when LedgerManager is closing), why we need additional layer for it?
>
> Ivan Kelly wrote:
> re: flags vs state, flags are state. Once you go past one flag, you should move to a more explicit state mechanism.
>
> You cannot do this with one state machine for the whole client. Each individual channel has it's own state, so each need their own state machine. In fact, pcbc already is a state machine.
>
> I think that state machines for bookie client and ledgermanager may be enough for now. Changing it for the whole client is a pretty mammoth task.
>
> fpj wrote:
> We need a state machine to represent the states the user sees, like starting, ready, closed. You may claim that this is the state machine of bookkeeper, but I would say that this state machine we expose reflects the state of the overall client. You can surely have a different state machine per component and map the states we expose to applications to states of the different components.
since fixing the close sequence with the state in bookie client and ledger manager would be enough and clear for resolving this problem here. introducing a whole state for overall client seems to be out of scope of this ticket. as it is unclear for me that what do starting, ready and closed state in bookkeeper level mean. if you want to introduce those states in bookkeeper level, could you create a separated task for it, which we could keep each task simple and clear?
- 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
>
>
Re: Review Request 17352: BOOKKEEPER-654: Bookkeeper client operations are
allowed even after its closure, bk#close()
Posted by fp...@apache.org.
> 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.
>
> Sijie Guo wrote:
> 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?
>
> fpj wrote:
> I like the idea of reasoning about this problem using state machines; however, I'm not convinced that considering four separate state machines is the right thing to do. We can think of an overall state machine for the client (e.g., CONNECTING, READY, CLOSED) and state machines per component.
>
> Sijie Guo wrote:
> is there any difference between using a flag than a state for the whole client? or stepping back, do we really need so many state for the whole client?
>
> will the state in ledger manager and the state in bookie client (pcbc) not be enough to define the state of the whole client?
It wouldn't be surprising if we can do with one state machine for the whole client. My thinking is that it might be useful for one global client to translate into multiple states for a component. For example, the state of the overall client might be CLOSED, meaning that the app can't submit new requests, but the state of BookieClient/PCBC might be CLOSING because we are still erroring out pending requests. Making such a distinction might not be strictly necessary, but it doesn't sound wrong to have it.
- fpj
-----------------------------------------------------------
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
>
>
Re: Review Request 17352: BOOKKEEPER-654: Bookkeeper client operations are
allowed even after its closure, bk#close()
Posted by fp...@apache.org.
> 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.
>
> Sijie Guo wrote:
> 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?
>
> fpj wrote:
> I like the idea of reasoning about this problem using state machines; however, I'm not convinced that considering four separate state machines is the right thing to do. We can think of an overall state machine for the client (e.g., CONNECTING, READY, CLOSED) and state machines per component.
>
> Sijie Guo wrote:
> is there any difference between using a flag than a state for the whole client? or stepping back, do we really need so many state for the whole client?
>
> will the state in ledger manager and the state in bookie client (pcbc) not be enough to define the state of the whole client?
>
> fpj wrote:
> It wouldn't be surprising if we can do with one state machine for the whole client. My thinking is that it might be useful for one global client to translate into multiple states for a component. For example, the state of the overall client might be CLOSED, meaning that the app can't submit new requests, but the state of BookieClient/PCBC might be CLOSING because we are still erroring out pending requests. Making such a distinction might not be strictly necessary, but it doesn't sound wrong to have it.
>
> Sijie Guo wrote:
> the reason that I only did for BookieClient(PCBC) and LedgerManager is for performance consideration. Since BookieClient(PCBC) and LedgerManager already defined the state of BookKeeper (All data operations will be rejected when BookieClient is closing, while all metadata operations will be rejected when LedgerManager is closing), why we need additional layer for it?
>
> Ivan Kelly wrote:
> re: flags vs state, flags are state. Once you go past one flag, you should move to a more explicit state mechanism.
>
> You cannot do this with one state machine for the whole client. Each individual channel has it's own state, so each need their own state machine. In fact, pcbc already is a state machine.
>
> I think that state machines for bookie client and ledgermanager may be enough for now. Changing it for the whole client is a pretty mammoth task.
>
> fpj wrote:
> We need a state machine to represent the states the user sees, like starting, ready, closed. You may claim that this is the state machine of bookkeeper, but I would say that this state machine we expose reflects the state of the overall client. You can surely have a different state machine per component and map the states we expose to applications to states of the different components.
>
> Sijie Guo wrote:
> since fixing the close sequence with the state in bookie client and ledger manager would be enough and clear for resolving this problem here. introducing a whole state for overall client seems to be out of scope of this ticket. as it is unclear for me that what do starting, ready and closed state in bookkeeper level mean. if you want to introduce those states in bookkeeper level, could you create a separated task for it, which we could keep each task simple and clear?
Sure, I'm feeling we are stuck here and it would be good to make progress. Just to be on the same page, even if we have a state machine for each component, they need to be interconnected somehow, since they are part of the same client and the behavior of one component influences the other, e.g., PCBC is not independent of BookieClient. Consequently, there should be implicit one state machine driving the whole computation of the client. An example of an overarching state machine like this is the ZK one for sessions:
http://zookeeper.apache.org/doc/r3.4.5/zookeeperProgrammers.html#ch_zkSessions
- fpj
-----------------------------------------------------------
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
>
>
Re: Review Request 17352: BOOKKEEPER-654: Bookkeeper client operations are
allowed even after its closure, bk#close()
Posted by Ivan Kelly <iv...@apache.org>.
-----------------------------------------------------------
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
>
>