You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@bookkeeper.apache.org by GitBox <gi...@apache.org> on 2020/03/07 03:03:58 UTC
[GitHub] [bookkeeper] rdhabalia opened a new pull request #2281:
[bookie-ledger-recovery] Fix bookie recovery stuck even with enough
ack-quorum response
rdhabalia opened a new pull request #2281: [bookie-ledger-recovery] Fix bookie recovery stuck even with enough ack-quorum response
URL: https://github.com/apache/bookkeeper/pull/2281
### Motivation
As discussed at https://github.com/apache/pulsar/issues/6505
Bk-client was not able to recover ledger which has 2 write/ack quorum and one of the bookie went down and recovery was kept failing and bookkeeper client was not able to recover the ledger.
**BK-Client log**
```
20:44:43.721 [BookKeeperClientWorker-OrderedExecutor-1-0] ERROR org.apache.bookkeeper.client.ReadLastConfirmedOp - While readLastConfirmed ledger: 1234567 did not hear success responses from all quorums
20:44:43.721 [bookkeeper-io-12-27] ERROR org.apache.bookkeeper.proto.PerChannelBookieClient - Could not connect to bookie: [id: 0xb8b97441, L:/1.1.1.1:1234]/1.1.1.2:3181, current s
tate CONNECTING :
io.netty.channel.AbstractChannel$AnnotatedConnectException: finishConnect(..) failed: No route to host: /1.1.1.2:3181
at io.netty.channel.unix.Errors.throwConnectException(Errors.java:112) ~[netty-all-4.1.32.Final.jar:4.1.32.Final]
at io.netty.channel.unix.Socket.finishConnect(Socket.java:269) ~[netty-all-4.1.32.Final.jar:4.1.32.Final]
at io.netty.channel.epoll.AbstractEpollChannel$AbstractEpollUnsafe.doFinishConnect(AbstractEpollChannel.java:665) [netty-transport-native-epoll-4.1.31.Final.jar:4.1.31.Final]
at io.netty.channel.epoll.AbstractEpollChannel$AbstractEpollUnsafe.finishConnect(AbstractEpollChannel.java:642) [netty-transport-native-epoll-4.1.31.Final.jar:4.1.31.Final]
at io.netty.channel.epoll.AbstractEpollChannel$AbstractEpollUnsafe.epollOutReady(AbstractEpollChannel.java:522) [netty-transport-native-epoll-4.1.31.Final.jar:4.1.31.Final]
at io.netty.channel.epoll.EpollEventLoop.processReady(EpollEventLoop.java:423) [netty-transport-native-epoll-4.1.31.Final.jar:4.1.31.Final]
at io.netty.channel.epoll.EpollEventLoop.run(EpollEventLoop.java:330) [netty-transport-native-epoll-4.1.31.Final.jar:4.1.31.Final]
at io.netty.util.concurrent.SingleThreadEventExecutor$5.run(SingleThreadEventExecutor.java:897) [netty-common-4.1.31.Final.jar:4.1.31.Final]
at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30) [netty-common-4.1.31.Final.jar:4.1.31.Final]
at java.lang.Thread.run(Thread.java:834) [?:?]
Caused by: java.net.ConnectException: finishConnect(..) failed: No route to host
```
**Ledger metadata**
```
BookieMetadataFormatVersion 2
quorumSize: 2
ensembleSize: 2
length: 0
lastEntryId: -1
state: IN_RECOVERY
segment {
ensembleMember: "1.1.1.1:3181"
ensembleMember: "1.1.1.2:3181"
firstEntryId: 0
}
digestType: CRC32
```
**Root cause:**
Bookie should be able to recover ledger once it receives the response from total N (`(Qw - Qa)+1`) bookies. But it was waiting for a successful response from both quorums.
Reference: https://bookkeeper.apache.org/docs/4.5.0/development/protocol/
### Modification
Bookie should be able to recover ledger once it receives the response from total N (`(Qw - Qa)+1`) bookies.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [bookkeeper] eolivelli commented on issue #2281:
[bookie-ledger-recovery] Fix bookie recovery stuck even with enough
ack-quorum response
Posted by GitBox <gi...@apache.org>.
eolivelli commented on issue #2281: [bookie-ledger-recovery] Fix bookie recovery stuck even with enough ack-quorum response
URL: https://github.com/apache/bookkeeper/pull/2281#issuecomment-596083925
Okay.
We are waiting for WQ - AQ +1 so:
- WQ=2 AQ=2 wait for 1, it is okay as all bookies have every entry
- WQ=2 AQ=1 wait for 2, it is okay as we have to check every bookie
So your fix work well according to the protocol .
Please add test cases
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [bookkeeper] sijie commented on issue #2281:
[bookie-ledger-recovery] Fix bookie recovery stuck even with enough
ack-quorum response
Posted by GitBox <gi...@apache.org>.
sijie commented on issue #2281: [bookie-ledger-recovery] Fix bookie recovery stuck even with enough ack-quorum response
URL: https://github.com/apache/bookkeeper/pull/2281#issuecomment-600903706
@eolivelli please take a look
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [bookkeeper] rdhabalia commented on a change in pull request #2281:
[bookie-ledger-recovery] Fix bookie recovery stuck even with enough
ack-quorum response
Posted by GitBox <gi...@apache.org>.
rdhabalia commented on a change in pull request #2281: [bookie-ledger-recovery] Fix bookie recovery stuck even with enough ack-quorum response
URL: https://github.com/apache/bookkeeper/pull/2281#discussion_r403332758
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/client/ReadLastConfirmedOp.java
##########
@@ -137,8 +140,15 @@ public synchronized void readEntryComplete(final int rc, final long ledgerId, fi
}
if (numResponsesPending == 0 && !completed) {
Review comment:
yes makes sense. provided test in https://github.com/apache/bookkeeper/commit/dcdd1e887ef01f7515053f7eb06f8479de8700ff is not correct and it should cover the coverage and if that covers the coverage then we won't need this fix.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [bookkeeper] rdhabalia commented on a change in pull request #2281:
[bookie-ledger-recovery] Fix bookie recovery stuck even with enough
ack-quorum response
Posted by GitBox <gi...@apache.org>.
rdhabalia commented on a change in pull request #2281: [bookie-ledger-recovery] Fix bookie recovery stuck even with enough ack-quorum response
URL: https://github.com/apache/bookkeeper/pull/2281#discussion_r390007211
##########
File path: bookkeeper-server/src/test/java/org/apache/bookkeeper/client/ParallelLedgerRecoveryTest.java
##########
@@ -693,4 +695,55 @@ public void readLastConfirmedDataComplete(int rc, DigestManager.RecoveryData dat
readBk.close();
}
+ /**
+ * Validate ledger can recover with response: (Qw - Qa)+1.
+ * @throws Exception
+ */
+ @Test
+ public void testRecoveryWithUnavailableBookie() throws Exception {
+
+ byte[] passwd = "recovery-when-closing-ledger-handle".getBytes(UTF_8);
+
+ int ensembleSize = 2;
+ int writeQuorumSize = 2;
+ int ackQuormSize = 2;
+ ClientConfiguration newConf = new ClientConfiguration();
+ newConf.addConfiguration(baseClientConf);
+ final BookKeeper newBk0 = new BookKeeper(newConf);
+ final LedgerHandle lh0 = newBk0.createLedger(ensembleSize, writeQuorumSize, ackQuormSize, digestType, passwd);
+ final BookKeeper readBk = new BookKeeper(newConf);
+ final LedgerHandle readLh = readBk.openLedgerNoRecovery(lh0.getId(), digestType, passwd);
+
+ MutableInt responseCode = new MutableInt(100);
+ CountDownLatch responseLatch = new CountDownLatch(1);
+ ReadLastConfirmedOp readLCOp = new ReadLastConfirmedOp(readLh, bkc.getBookieClient(),
+ readLh.getLedgerMetadata().getAllEnsembles().lastEntry().getValue(),
+ new ReadLastConfirmedOp.LastConfirmedDataCallback() {
+ @Override
+ public void readLastConfirmedDataComplete(int rc, DigestManager.RecoveryData data) {
+ responseCode.setValue(rc);
+ responseLatch.countDown();
+ }
+ });
+
+ // complete first successful write.
+ byte[] lac = new byte[DigestManager.METADATA_LENGTH * 2];
+ try {
+ readLCOp.readEntryComplete(BKException.Code.OK, 0, 0, Unpooled.wrappedBuffer(lac, 0, lac.length), 0);
+ } catch (Exception e) {
+ // Ok
Review comment:
actually it was failing due to digest verification because of mock data but I have fixed it after spending some time understanding it correctly.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [bookkeeper] eolivelli commented on issue #2281:
[bookie-ledger-recovery] Fix bookie recovery stuck even with enough
ack-quorum response
Posted by GitBox <gi...@apache.org>.
eolivelli commented on issue #2281: [bookie-ledger-recovery] Fix bookie recovery stuck even with enough ack-quorum response
URL: https://github.com/apache/bookkeeper/pull/2281#issuecomment-596061151
This is related to #2273
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [bookkeeper] eolivelli commented on issue #2281:
[bookie-ledger-recovery] Fix bookie recovery stuck even with enough
ack-quorum response
Posted by GitBox <gi...@apache.org>.
eolivelli commented on issue #2281: [bookie-ledger-recovery] Fix bookie recovery stuck even with enough ack-quorum response
URL: https://github.com/apache/bookkeeper/pull/2281#issuecomment-602695782
@rdhabalia I have merged this patch to master branch.
Thank you !
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [bookkeeper] eolivelli merged pull request #2281:
[bookie-ledger-recovery] Fix bookie recovery stuck even with enough
ack-quorum response
Posted by GitBox <gi...@apache.org>.
eolivelli merged pull request #2281: [bookie-ledger-recovery] Fix bookie recovery stuck even with enough ack-quorum response
URL: https://github.com/apache/bookkeeper/pull/2281
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [bookkeeper] eolivelli commented on issue #2281:
[bookie-ledger-recovery] Fix bookie recovery stuck even with enough
ack-quorum response
Posted by GitBox <gi...@apache.org>.
eolivelli commented on issue #2281: [bookie-ledger-recovery] Fix bookie recovery stuck even with enough ack-quorum response
URL: https://github.com/apache/bookkeeper/pull/2281#issuecomment-602539631
I will merge today if @diegosalvi do not have other comments
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [bookkeeper] eolivelli commented on a change in pull request #2281:
[bookie-ledger-recovery] Fix bookie recovery stuck even with enough
ack-quorum response
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #2281: [bookie-ledger-recovery] Fix bookie recovery stuck even with enough ack-quorum response
URL: https://github.com/apache/bookkeeper/pull/2281#discussion_r389238677
##########
File path: bookkeeper-server/src/test/java/org/apache/bookkeeper/client/ParallelLedgerRecoveryTest.java
##########
@@ -693,4 +695,55 @@ public void readLastConfirmedDataComplete(int rc, DigestManager.RecoveryData dat
readBk.close();
}
+ /**
+ * Validate ledger can recover with response: (Qw - Qa)+1.
+ * @throws Exception
+ */
+ @Test
+ public void testRecoveryWithUnavailableBookie() throws Exception {
+
+ byte[] passwd = "recovery-when-closing-ledger-handle".getBytes(UTF_8);
+
+ int ensembleSize = 2;
+ int writeQuorumSize = 2;
+ int ackQuormSize = 2;
+ ClientConfiguration newConf = new ClientConfiguration();
+ newConf.addConfiguration(baseClientConf);
+ final BookKeeper newBk0 = new BookKeeper(newConf);
+ final LedgerHandle lh0 = newBk0.createLedger(ensembleSize, writeQuorumSize, ackQuormSize, digestType, passwd);
+ final BookKeeper readBk = new BookKeeper(newConf);
+ final LedgerHandle readLh = readBk.openLedgerNoRecovery(lh0.getId(), digestType, passwd);
+
+ MutableInt responseCode = new MutableInt(100);
+ CountDownLatch responseLatch = new CountDownLatch(1);
+ ReadLastConfirmedOp readLCOp = new ReadLastConfirmedOp(readLh, bkc.getBookieClient(),
+ readLh.getLedgerMetadata().getAllEnsembles().lastEntry().getValue(),
+ new ReadLastConfirmedOp.LastConfirmedDataCallback() {
+ @Override
+ public void readLastConfirmedDataComplete(int rc, DigestManager.RecoveryData data) {
+ responseCode.setValue(rc);
+ responseLatch.countDown();
+ }
+ });
+
+ // complete first successful write.
+ byte[] lac = new byte[DigestManager.METADATA_LENGTH * 2];
+ try {
+ readLCOp.readEntryComplete(BKException.Code.OK, 0, 0, Unpooled.wrappedBuffer(lac, 0, lac.length), 0);
+ } catch (Exception e) {
+ // Ok
Review comment:
What is this expected error?
Can we add an assertion about this exception?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [bookkeeper] ivankelly commented on a change in pull request #2281:
[bookie-ledger-recovery] Fix bookie recovery stuck even with enough
ack-quorum response
Posted by GitBox <gi...@apache.org>.
ivankelly commented on a change in pull request #2281: [bookie-ledger-recovery] Fix bookie recovery stuck even with enough ack-quorum response
URL: https://github.com/apache/bookkeeper/pull/2281#discussion_r403306604
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/client/ReadLastConfirmedOp.java
##########
@@ -137,8 +140,15 @@ public synchronized void readEntryComplete(final int rc, final long ledgerId, fi
}
if (numResponsesPending == 0 && !completed) {
Review comment:
@eolivelli @sijie
This fix is the wrong fix. This problem is the same problem as #2273 , and we've also seen internally. The root cause is that quorum coverage is broken. 5e399df broke it. 5e399df was a change in the write path, so why it touched the quorum coverage in the read path is a mystery. dcdd1e88 supplemented the break which and added a test case. The test case tests exactly the wrong behaviour Pre-5e399df it worked correctly.
It seems there is confusion as to what the quorum coverage is supposed to do.
The quorum coverage set is there to check if we have received a valid response from enough nodes that there cannot exist an entry which has been acknowledged to the client which has not touched at least one of these nodes. In other words, there cannot exist an entry which formed an ack quorum without touching one of the nodes we have talked to.
The quorum coverage set should only have two states for a node, known or unknown. The known state means that we know definitely than an entry does or does not exist on that node. So, an OK, NoSuchLedger and NoSuchEntry response will move the node into a known state. Anything else is an unknown state. No response from node is unknown state. Node had an IOException is an unknown state.
Take the test supplied with dcdd1e88:
```
RoundRobinDistributionSchedule schedule = new RoundRobinDistributionSchedule(
5, 3, 5);
DistributionSchedule.QuorumCoverageSet covSet = schedule.getCoverageSet();
covSet.addBookie(0, BKException.Code.NoSuchLedgerExistsException);
covSet.addBookie(1, BKException.Code.NoSuchEntryException);
covSet.addBookie(2, BKException.Code.NoSuchLedgerExistsException);
covSet.addBookie(3, BKException.Code.UNINITIALIZED);
covSet.addBookie(4, BKException.Code.UNINITIALIZED);
assertFalse(covSet.checkCovered());
```
For clarify, w is 5, a is 3, e is 5. RoundRobinDistributionSchedule parameters are messed up.
This test is simply wrong. The coverage set is covered. There is no way an entry can have ack quorum in any write quorum. There is no set of 3 bookies in the unknown state.
Anyhow, I have a fix going through CI internally. I'll push up a PR on monday.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [bookkeeper] rdhabalia commented on issue #2281:
[bookie-ledger-recovery] Fix bookie recovery stuck even with enough
ack-quorum response
Posted by GitBox <gi...@apache.org>.
rdhabalia commented on issue #2281: [bookie-ledger-recovery] Fix bookie recovery stuck even with enough ack-quorum response
URL: https://github.com/apache/bookkeeper/pull/2281#issuecomment-596821669
@eolivelli @diegosalvi I have spent some time to cover most of the usecases with multiple groups of tests.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services