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/01/26 20:26:03 UTC
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/
-----------------------------------------------------------
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 cb3bb26
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerRecoveryOp.java 4625bbb
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 4466ce3
bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieProtocol.java bc1cfb0
bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieServer.java beab5e8
bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PerChannelBookieClient.java 2fa79bb
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/BookieRecoveryTest.java 8526db5
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
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-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 fp...@apache.org.
> On 2012-01-28 11:00:01, fpj wrote:
> > bookkeeper-server/src/test/java/org/apache/bookkeeper/client/TestFencing.java, line 370
> > <https://reviews.apache.org/r/3642/diff/1/?file=70865#file70865line370>
> >
> > I don't think we currently throw this exception anywhere in the code (BKLedgerRecoveryException). It is nowhere else in this patch and it is not present in trunk. I think we are throwing an exception here, but not the the right one.
> >
My bad, I just realized that we do it here in LedgerOpenOp:
public void operationComplete(int rc, Void result) {
if (rc != BKException.Code.OK) {
cb.openComplete(BKException.Code.LedgerRecoveryException, null, LedgerOpenOp.this.ctx);
} else {
cb.openComplete(BKException.Code.OK, lh, LedgerOpenOp.this.ctx);
}
}
> On 2012-01-28 11:00:01, fpj wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PerChannelBookieClient.java, line 293
> > <https://reviews.apache.org/r/3642/diff/1/?file=70863#file70863line293>
> >
> > Is this where we fail the openLedger? If so, by checking the method, it doesn't seem to be using the right code. This is related to my other comment on the exception not being the right one.
It sounds like in LedgerRecoveryOp we just pass the error code to LedgerOpenOp and if it is not OK, we make it a LedgerRecoveryException. Consequently, it works to return BKException.Code.BookieHandleNotAvailableException in PerChannelBookieClient.errorOutReadKey(). The error here is not BookieHandleNotAvailable. Should we fix this one here?
- fpj
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3642/#review4668
-----------------------------------------------------------
On 2012-01-26 19:26:03, Ivan Kelly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3642/
> -----------------------------------------------------------
>
> (Updated 2012-01-26 19:26:03)
>
>
> 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 cb3bb26
> bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerRecoveryOp.java 4625bbb
> 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 4466ce3
> bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieProtocol.java bc1cfb0
> bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieServer.java beab5e8
> bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PerChannelBookieClient.java 2fa79bb
> bookkeeper-server/src/test/java/org/apache/bookkeeper/client/BookieRecoveryTest.java 8526db5
> 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 fp...@apache.org.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3642/#review4668
-----------------------------------------------------------
If I understand the overall flow right, giving the wrong password will make readEntryAndDoFencing fail in the initiate call of the recovery procedure. It fails because the bookie server checks and returns an error (IOException in BookieServer). I don't fully understand the flow once we determine that the password is incorrect based on the two comments below.
bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PerChannelBookieClient.java
<https://reviews.apache.org/r/3642/#comment10387>
Is this where we fail the openLedger? If so, by checking the method, it doesn't seem to be using the right code. This is related to my other comment on the exception not being the right one.
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/TestFencing.java
<https://reviews.apache.org/r/3642/#comment10385>
I don't think we currently throw this exception anywhere in the code (BKLedgerRecoveryException). It is nowhere else in this patch and it is not present in trunk. I think we are throwing an exception here, but not the the right one.
- fpj
On 2012-01-26 19:26:03, Ivan Kelly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3642/
> -----------------------------------------------------------
>
> (Updated 2012-01-26 19:26:03)
>
>
> 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 cb3bb26
> bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerRecoveryOp.java 4625bbb
> 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 4466ce3
> bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieProtocol.java bc1cfb0
> bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieServer.java beab5e8
> bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PerChannelBookieClient.java 2fa79bb
> bookkeeper-server/src/test/java/org/apache/bookkeeper/client/BookieRecoveryTest.java 8526db5
> 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 fp...@apache.org.
> On 2012-01-30 10:06:44, Ivan Kelly wrote:
> > I'm thinking I should add something more explicit to allow the client see that the actual issue is the wrong password, maybe by throwing a different exception, or logging a very explicit error message.
> >
Sounds like a good idea, since LedgerRecoveryException is not precise enough. The ReadException is also mixing wrong password and read failure, right?
> On 2012-01-30 10:06:44, Ivan Kelly wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PerChannelBookieClient.java, line 293
> > <https://reviews.apache.org/r/3642/diff/1/?file=70863#file70863line293>
> >
> > This isn't were the error would come from in the case of wrong password for fencing. Listener here is only for writing to the socket. Once you've written to the socket the callback is invoked.
> >
> > For wrong password errors, we have to wait for the response from the bookie, which is handled in #messageReceived, and in this case by #handleReadResponse, which will interpret the rc as a ReadException. When LedgerRecoveryOp#readEntryComplete receives ReadException for all of the read requests, it throws a LedgerRecoveryException to the client.
Got it, seems right to me.
- fpj
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3642/#review4685
-----------------------------------------------------------
On 2012-01-26 19:26:03, Ivan Kelly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3642/
> -----------------------------------------------------------
>
> (Updated 2012-01-26 19:26:03)
>
>
> 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 cb3bb26
> bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerRecoveryOp.java 4625bbb
> 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 4466ce3
> bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieProtocol.java bc1cfb0
> bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieServer.java beab5e8
> bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PerChannelBookieClient.java 2fa79bb
> bookkeeper-server/src/test/java/org/apache/bookkeeper/client/BookieRecoveryTest.java 8526db5
> 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/#review4685
-----------------------------------------------------------
I'm thinking I should add something more explicit to allow the client see that the actual issue is the wrong password, maybe by throwing a different exception, or logging a very explicit error message.
bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PerChannelBookieClient.java
<https://reviews.apache.org/r/3642/#comment10426>
This isn't were the error would come from in the case of wrong password for fencing. Listener here is only for writing to the socket. Once you've written to the socket the callback is invoked.
For wrong password errors, we have to wait for the response from the bookie, which is handled in #messageReceived, and in this case by #handleReadResponse, which will interpret the rc as a ReadException. When LedgerRecoveryOp#readEntryComplete receives ReadException for all of the read requests, it throws a LedgerRecoveryException to the client.
- Ivan
On 2012-01-26 19:26:03, Ivan Kelly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3642/
> -----------------------------------------------------------
>
> (Updated 2012-01-26 19:26:03)
>
>
> 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 cb3bb26
> bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerRecoveryOp.java 4625bbb
> 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 4466ce3
> bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieProtocol.java bc1cfb0
> bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieServer.java beab5e8
> bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PerChannelBookieClient.java 2fa79bb
> bookkeeper-server/src/test/java/org/apache/bookkeeper/client/BookieRecoveryTest.java 8526db5
> 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
>
>