You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@bookkeeper.apache.org by Ivan Kelly <iv...@apache.org> on 2012/02/10 19:21:40 UTC
Re: Review Request: BOOKKEEPER-135 Fencing does not check the ledger
masterPasswd
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3642/
-----------------------------------------------------------
(Updated 2012-02-10 18:21:40.588207)
Review request for bookkeeper.
Summary
-------
When fencing, the ledger handle is not checked before the fencing is applied. Currently the openLedger does fail, on because it will addEntry and fail at that point, but by this stage, fencing has already been applied. The check should be earlier.
This addresses bug BOOKKEEPER-135.
https://issues.apache.org/jira/browse/BOOKKEEPER-135
Diffs (updated)
-----
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Bookie.java 57a6c29
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/LedgerCache.java 3e96d46
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/LedgerDescriptor.java 9da4aec
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BKException.java 911c660
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerOpenOp.java 56186ab
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerRecoveryOp.java 4625bbb
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/PendingAddOp.java 7aad751
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/PendingReadOp.java 29070eb
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/ReadLastConfirmedOp.java 43e999d
bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieClient.java 8a32c64
bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieProtocol.java bc1cfb0
bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieServer.java 7217da6
bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PerChannelBookieClient.java a68fc8c
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/BookieRecoveryTest.java cbd2277
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/TestFencing.java 015e4e4
bookkeeper-server/src/test/java/org/apache/bookkeeper/test/BookieClientTest.java 99d6ef0
Diff: https://reviews.apache.org/r/3642/diff
Testing
-------
Thanks,
Ivan
Re: Review Request: BOOKKEEPER-135 Fencing does not check the ledger
masterPasswd
Posted by Ivan Kelly <iv...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3642/
-----------------------------------------------------------
(Updated 2012-03-30 16:58:23.130753)
Review request for bookkeeper.
Changes
-------
Addressed Sijie's comments
Summary
-------
When fencing, the ledger handle is not checked before the fencing is applied. Currently the openLedger does fail, on because it will addEntry and fail at that point, but by this stage, fencing has already been applied. The check should be earlier.
This addresses bug BOOKKEEPER-135.
https://issues.apache.org/jira/browse/BOOKKEEPER-135
Diffs (updated)
-----
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Bookie.java ee38862
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BKException.java 911c660
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerOpenOp.java 56186ab
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerRecoveryOp.java c67a79c
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/PendingAddOp.java 7aad751
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/PendingReadOp.java 539d6b2
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/ReadLastConfirmedOp.java 7dd5363
bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieClient.java 8a32c64
bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieProtocol.java 8598c08
bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieServer.java c5c1422
bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PerChannelBookieClient.java 75a8e8c
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/BookieRecoveryTest.java b8923e8
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/TestFencing.java 7de1c10
bookkeeper-server/src/test/java/org/apache/bookkeeper/test/BookieClientTest.java 99d6ef0
Diff: https://reviews.apache.org/r/3642/diff
Testing
-------
Thanks,
Ivan
Re: Review Request: BOOKKEEPER-135 Fencing does not check the ledger
masterPasswd
Posted by Ivan Kelly <iv...@apache.org>.
> On 2012-03-29 14:10:15, Sijie Guo wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieServer.java, line 385
> > <https://reviews.apache.org/r/3642/diff/4/?file=93827#file93827line385>
> >
> > just curious, do we need any cases to test different version compatibility?
I think this would be very hard to test, as it would mean either pulling down a different version of the code, or duplicating the old code in our tests. It's possible, but the benefit vs payoff is very little.
- Ivan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3642/#review6519
-----------------------------------------------------------
On 2012-03-20 18:13:56, Ivan Kelly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3642/
> -----------------------------------------------------------
>
> (Updated 2012-03-20 18:13:56)
>
>
> Review request for bookkeeper.
>
>
> Summary
> -------
>
> When fencing, the ledger handle is not checked before the fencing is applied. Currently the openLedger does fail, on because it will addEntry and fail at that point, but by this stage, fencing has already been applied. The check should be earlier.
>
>
> This addresses bug BOOKKEEPER-135.
> https://issues.apache.org/jira/browse/BOOKKEEPER-135
>
>
> Diffs
> -----
>
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Bookie.java ad41ba5
> bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BKException.java 911c660
> bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerOpenOp.java 56186ab
> bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerRecoveryOp.java c67a79c
> bookkeeper-server/src/main/java/org/apache/bookkeeper/client/PendingAddOp.java 7aad751
> bookkeeper-server/src/main/java/org/apache/bookkeeper/client/PendingReadOp.java 539d6b2
> bookkeeper-server/src/main/java/org/apache/bookkeeper/client/ReadLastConfirmedOp.java 7dd5363
> bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieClient.java 8a32c64
> bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieProtocol.java 8598c08
> bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieServer.java 1a315e1
> bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PerChannelBookieClient.java 75a8e8c
> bookkeeper-server/src/test/java/org/apache/bookkeeper/client/BookieRecoveryTest.java b8923e8
> bookkeeper-server/src/test/java/org/apache/bookkeeper/client/TestFencing.java 7de1c10
> bookkeeper-server/src/test/java/org/apache/bookkeeper/test/BookieClientTest.java 99d6ef0
>
> Diff: https://reviews.apache.org/r/3642/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ivan
>
>
Re: Review Request: BOOKKEEPER-135 Fencing does not check the ledger
masterPasswd
Posted by Sijie Guo <gu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3642/#review6519
-----------------------------------------------------------
the whole patch is very good. I just have some slight comments as below.
bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieServer.java
<https://reviews.apache.org/r/3642/#comment14182>
just curious, do we need any cases to test different version compatibility?
bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieServer.java
<https://reviews.apache.org/r/3642/#comment14183>
it would better to define 20 as constant, since there is several places referencing it.
bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieServer.java
<https://reviews.apache.org/r/3642/#comment14181>
it would better to throw a BookieException, so it would be caught as EUA error.
- Sijie
On 2012-03-20 18:13:56, Ivan Kelly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3642/
> -----------------------------------------------------------
>
> (Updated 2012-03-20 18:13:56)
>
>
> Review request for bookkeeper.
>
>
> Summary
> -------
>
> When fencing, the ledger handle is not checked before the fencing is applied. Currently the openLedger does fail, on because it will addEntry and fail at that point, but by this stage, fencing has already been applied. The check should be earlier.
>
>
> This addresses bug BOOKKEEPER-135.
> https://issues.apache.org/jira/browse/BOOKKEEPER-135
>
>
> Diffs
> -----
>
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Bookie.java ad41ba5
> bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BKException.java 911c660
> bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerOpenOp.java 56186ab
> bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerRecoveryOp.java c67a79c
> bookkeeper-server/src/main/java/org/apache/bookkeeper/client/PendingAddOp.java 7aad751
> bookkeeper-server/src/main/java/org/apache/bookkeeper/client/PendingReadOp.java 539d6b2
> bookkeeper-server/src/main/java/org/apache/bookkeeper/client/ReadLastConfirmedOp.java 7dd5363
> bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieClient.java 8a32c64
> bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieProtocol.java 8598c08
> bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieServer.java 1a315e1
> bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PerChannelBookieClient.java 75a8e8c
> bookkeeper-server/src/test/java/org/apache/bookkeeper/client/BookieRecoveryTest.java b8923e8
> bookkeeper-server/src/test/java/org/apache/bookkeeper/client/TestFencing.java 7de1c10
> bookkeeper-server/src/test/java/org/apache/bookkeeper/test/BookieClientTest.java 99d6ef0
>
> Diff: https://reviews.apache.org/r/3642/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ivan
>
>
Re: Review Request: BOOKKEEPER-135 Fencing does not check the ledger
masterPasswd
Posted by Ivan Kelly <iv...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3642/
-----------------------------------------------------------
(Updated 2012-03-20 18:13:56.162926)
Review request for bookkeeper.
Changes
-------
New patch is much simpler on the server side. A lot of the changes are just added the new error path for unauthorized fencing.
Summary
-------
When fencing, the ledger handle is not checked before the fencing is applied. Currently the openLedger does fail, on because it will addEntry and fail at that point, but by this stage, fencing has already been applied. The check should be earlier.
This addresses bug BOOKKEEPER-135.
https://issues.apache.org/jira/browse/BOOKKEEPER-135
Diffs (updated)
-----
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Bookie.java ad41ba5
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BKException.java 911c660
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerOpenOp.java 56186ab
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerRecoveryOp.java c67a79c
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/PendingAddOp.java 7aad751
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/PendingReadOp.java 539d6b2
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/ReadLastConfirmedOp.java 7dd5363
bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieClient.java 8a32c64
bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieProtocol.java 8598c08
bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieServer.java 1a315e1
bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PerChannelBookieClient.java 75a8e8c
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/BookieRecoveryTest.java b8923e8
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/TestFencing.java 7de1c10
bookkeeper-server/src/test/java/org/apache/bookkeeper/test/BookieClientTest.java 99d6ef0
Diff: https://reviews.apache.org/r/3642/diff
Testing
-------
Thanks,
Ivan
Re: Review Request: BOOKKEEPER-135 Fencing does not check the ledger
masterPasswd
Posted by Ivan Kelly <iv...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3642/
-----------------------------------------------------------
(Updated 2012-02-16 16:51:41.014079)
Review request for bookkeeper.
Summary
-------
When fencing, the ledger handle is not checked before the fencing is applied. Currently the openLedger does fail, on because it will addEntry and fail at that point, but by this stage, fencing has already been applied. The check should be earlier.
This addresses bug BOOKKEEPER-135.
https://issues.apache.org/jira/browse/BOOKKEEPER-135
Diffs (updated)
-----
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Bookie.java 57a6c29
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/LedgerCache.java 3e96d46
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/LedgerDescriptor.java 9da4aec
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BKException.java 911c660
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerOpenOp.java 56186ab
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerRecoveryOp.java c67a79c
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/PendingAddOp.java 7aad751
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/PendingReadOp.java 29070eb
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/ReadLastConfirmedOp.java 7dd5363
bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieClient.java 8a32c64
bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieProtocol.java 8598c08
bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieServer.java 7217da6
bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PerChannelBookieClient.java ca055e8
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/BookieRecoveryTest.java 99258ac
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/TestFencing.java 015e4e4
bookkeeper-server/src/test/java/org/apache/bookkeeper/test/BookieClientTest.java 99d6ef0
Diff: https://reviews.apache.org/r/3642/diff
Testing
-------
Thanks,
Ivan