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/02/02 07:34:45 UTC
Re: 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/
-----------------------------------------------------------
(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 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