This is an automated email from the ASF dual-hosted git repository.
eolivelli pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/bookkeeper.git
The following commit(s) were added to refs/heads/master by this push:
new d4d0060 ISSUE #2506: Skip unavailable bookies during
verifyLedgerFragment
d4d0060 is described below
commit d4d0060bc38995777da5cc2363c7af897a908f27
Author: Michael Marshall <[email protected]>
AuthorDate: Tue Feb 16 04:19:02 2021 -0700
ISSUE #2506: Skip unavailable bookies during verifyLedgerFragment
Descriptions of the changes in this PR:
This PR improves the `verifyLedgerFragment` method in the `LedgerChecker`
by skipping calls to bookies that are known to be unavailable. The "bad
bookies" are calculated by using metadata available in ZK and accessed by the
`BookKeeperAdmin`. Note that `verifyLedgerFragment` will still run checks on
all other bookies that appear available.
### Motivation
The motivation for this change is demonstrated in #2506. As this code
currently works, there are a ton of calls made to unavailable bookies with the
intent of calculating bad (unavailable) bookies. This proposed change would
greatly decrease the number of calls that the auditor and the replicator need
to make to calculate which ledgers need replicating.
### Changes
1. Added `getUnavailableBookies` method to the `BookKeeperAdmin`. This
method could attempt to use caching, but it's not actually called that often,
so I think caching might not add complexity without much value.
2. Updated `verifyLedgerFragment` method signature to take a collection of
`unavailableBookies`.
Master Issue: #2506
### Testing
If these changes are acceptable, I'd like some help identifying the best
way to test these changes. I already added some coverage for the
`getUnavailableBookies` method, but I haven't explicitly tested the fundamental
change this PR proposes. Thanks!
Reviewers: Enrico Olivelli <[email protected]>, Andrey Yegorov
<[email protected]>
This closes #2597 from michaeljmarshall/leverage-available-bookies, closes
#2506
---
.../apache/bookkeeper/client/BookieWatcher.java | 9 ++++
.../bookkeeper/client/BookieWatcherImpl.java | 16 +++++++
.../apache/bookkeeper/client/LedgerChecker.java | 11 ++++-
.../bookkeeper/client/TestBookieWatcher.java | 56 ++++++++++++++++++++++
.../bookkeeper/client/TestLedgerChecker.java | 29 +++++++++++
.../replication/BookieAutoRecoveryTest.java | 4 +-
6 files changed, 122 insertions(+), 3 deletions(-)
diff --git
a/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookieWatcher.java
b/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookieWatcher.java
index 07b1695..fffe3eb 100644
---
a/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookieWatcher.java
+++
b/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookieWatcher.java
@@ -35,6 +35,15 @@ public interface BookieWatcher {
BookieAddressResolver getBookieAddressResolver();
/**
+ * Determine if a bookie should be considered unavailable.
+ *
+ * @param id
+ * Bookie to check
+ * @return whether or not the given bookie is unavailable
+ */
+ boolean isBookieUnavailable(BookieId id);
+
+ /**
* Create an ensemble with given <i>ensembleSize</i> and
<i>writeQuorumSize</i>.
*
* @param ensembleSize
diff --git
a/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookieWatcherImpl.java
b/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookieWatcherImpl.java
index d68ded7..26b741b 100644
---
a/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookieWatcherImpl.java
+++
b/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookieWatcherImpl.java
@@ -173,6 +173,22 @@ class BookieWatcherImpl implements BookieWatcher {
}
}
+ /**
+ * Determine if a bookie should be considered unavailable.
+ * This does not require a network call because this class
+ * maintains a current view of readonly and writable bookies.
+ * An unavailable bookie is one that is neither read only nor
+ * writable.
+ *
+ * @param id
+ * Bookie to check
+ * @return whether or not the given bookie is unavailable
+ */
+ @Override
+ public boolean isBookieUnavailable(BookieId id) {
+ return !readOnlyBookies.contains(id) && !writableBookies.contains(id);
+ }
+
// this callback is already not executed in zookeeper thread
private synchronized void processWritableBookiesChanged(Set<BookieId>
newBookieAddrs) {
// Update watcher outside ZK callback thread, to avoid deadlock in
case some other
diff --git
a/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerChecker.java
b/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerChecker.java
index a0a9bb2..f04ce24 100644
---
a/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerChecker.java
+++
b/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerChecker.java
@@ -49,6 +49,7 @@ public class LedgerChecker {
private static final Logger LOG =
LoggerFactory.getLogger(LedgerChecker.class);
public final BookieClient bookieClient;
+ public final BookieWatcher bookieWatcher;
static class InvalidFragmentException extends Exception {
private static final long serialVersionUID = 1467201276417062353L;
@@ -136,7 +137,12 @@ public class LedgerChecker {
}
public LedgerChecker(BookKeeper bkc) {
- bookieClient = bkc.getBookieClient();
+ this(bkc.getBookieClient(), bkc.getBookieWatcher());
+ }
+
+ public LedgerChecker(BookieClient client, BookieWatcher watcher) {
+ bookieClient = client;
+ bookieWatcher = watcher;
}
/**
@@ -197,6 +203,9 @@ public class LedgerChecker {
throw new InvalidFragmentException();
}
cb.operationComplete(BKException.Code.OK, fragment);
+ } else if
(bookieWatcher.isBookieUnavailable(fragment.getAddress(bookieIndex))) {
+ // fragment is on this bookie, but already know it's unavailable,
so skip the call
+
cb.operationComplete(BKException.Code.BookieHandleNotAvailableException,
fragment);
} else if (firstStored == lastStored) {
ReadManyEntriesCallback manycb = new ReadManyEntriesCallback(1,
fragment, cb);
diff --git
a/bookkeeper-server/src/test/java/org/apache/bookkeeper/client/TestBookieWatcher.java
b/bookkeeper-server/src/test/java/org/apache/bookkeeper/client/TestBookieWatcher.java
index 69fe345..93077af 100644
---
a/bookkeeper-server/src/test/java/org/apache/bookkeeper/client/TestBookieWatcher.java
+++
b/bookkeeper-server/src/test/java/org/apache/bookkeeper/client/TestBookieWatcher.java
@@ -23,12 +23,15 @@ package org.apache.bookkeeper.client;
import static org.junit.Assert.fail;
import java.io.IOException;
+import java.util.Collections;
+import java.util.Set;
import java.util.concurrent.CountDownLatch;
import java.util.concurrent.TimeUnit;
import lombok.Cleanup;
import org.apache.bookkeeper.conf.ClientConfiguration;
+import org.apache.bookkeeper.net.BookieId;
import org.apache.bookkeeper.test.BookKeeperClusterTestCase;
import org.apache.bookkeeper.zookeeper.ZooKeeperClient;
import org.apache.zookeeper.KeeperException;
@@ -37,6 +40,7 @@ import org.apache.zookeeper.Watcher;
import org.apache.zookeeper.Watcher.Event.EventType;
import org.apache.zookeeper.Watcher.Event.KeeperState;
import org.apache.zookeeper.ZooKeeper;
+import org.junit.Assert;
import org.junit.Test;
/**
@@ -68,6 +72,58 @@ public class TestBookieWatcher extends
BookKeeperClusterTestCase {
newZk.close();
}
+ /**
+ * Test to validate behavior of the isBookieUnavailable method.
+ * Because the method relies on getBookies and getReadOnlyBookies,
+ * these methods are essentially tested here as well.
+ *
+ * @throws Exception
+ */
+ @Test
+ public void testBookieWatcherIsBookieUnavailable() throws Exception {
+ BookieWatcher bookieWatcher = bkc.getBookieWatcher();
+
+ Set<BookieId> writableBookies1 = bookieWatcher.getBookies();
+ Set<BookieId> readonlyBookies1 = bookieWatcher.getReadOnlyBookies();
+
+ Assert.assertEquals("There should be writable bookies initially.", 2,
writableBookies1.size());
+ Assert.assertEquals("There should be no read only bookies initially.",
+ Collections.emptySet(), readonlyBookies1);
+
+ BookieId bookieId0 = bs.get(0).getBookieId();
+ BookieId bookieId1 = bs.get(1).getBookieId();
+
+ boolean isUnavailable1 = bookieWatcher.isBookieUnavailable(bookieId0);
+ Assert.assertFalse("The bookie should not be unavailable.",
isUnavailable1);
+
+ // Next, set to read only, which is still available
+ setBookieToReadOnly(bookieId0);
+
+ Set<BookieId> writableBookies2 = bookieWatcher.getBookies();
+ Set<BookieId> readonlyBookies2 = bookieWatcher.getReadOnlyBookies();
+
+ Assert.assertEquals("There should be one writable bookie.",
+ Collections.singleton(bookieId1), writableBookies2);
+ Assert.assertEquals("There should be one read only bookie.",
+ Collections.singleton(bookieId0), readonlyBookies2);
+
+ boolean isUnavailable2 = bookieWatcher.isBookieUnavailable(bookieId0);
+ Assert.assertFalse("The bookie should not be unavailable.",
isUnavailable2);
+
+ // Next, kill it, which should make it unavailable
+ killBookieAndWaitForZK(0);
+
+ Set<BookieId> writableBookies3 = bookieWatcher.getBookies();
+ Set<BookieId> readonlyBookies3 = bookieWatcher.getReadOnlyBookies();
+
+ Assert.assertEquals("There should be one writable bookie.",
+ Collections.singleton(bookieId1), writableBookies3);
+ Assert.assertEquals("There should be no read only bookies.",
Collections.emptySet(), readonlyBookies3);
+
+ boolean isUnavailable3 = bookieWatcher.isBookieUnavailable(bookieId0);
+ Assert.assertTrue("The bookie should be unavailable.", isUnavailable3);
+ }
+
@Test
public void testBookieWatcherSurviveWhenSessionExpired() throws Exception {
final int timeout = 2000;
diff --git
a/bookkeeper-server/src/test/java/org/apache/bookkeeper/client/TestLedgerChecker.java
b/bookkeeper-server/src/test/java/org/apache/bookkeeper/client/TestLedgerChecker.java
index de86776..9da4cb3 100644
---
a/bookkeeper-server/src/test/java/org/apache/bookkeeper/client/TestLedgerChecker.java
+++
b/bookkeeper-server/src/test/java/org/apache/bookkeeper/client/TestLedgerChecker.java
@@ -23,6 +23,11 @@ package org.apache.bookkeeper.client;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertTrue;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
import java.util.List;
import java.util.Set;
@@ -32,6 +37,7 @@ import org.apache.bookkeeper.net.BookieId;
import org.apache.bookkeeper.proto.BookkeeperInternalCallbacks.GenericCallback;
import org.apache.bookkeeper.test.BookKeeperClusterTestCase;
import org.junit.Test;
+
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -487,6 +493,29 @@ public class TestLedgerChecker extends
BookKeeperClusterTestCase {
lh1.close();
}
+ @Test
+ public void testVerifyLedgerFragmentSkipsUnavailableBookie() throws
Exception {
+ // Initialize LedgerChecker with mocked watcher to validate
interactions
+ BookieWatcher bookieWatcher = mock(BookieWatcher.class);
+ when(bookieWatcher.isBookieUnavailable(any())).thenReturn(true);
+ LedgerChecker mockedChecker = new LedgerChecker(bkc.getBookieClient(),
bookieWatcher);
+
+ LedgerHandle ledgerHandle =
bkc.createLedger(BookKeeper.DigestType.CRC32, TEST_LEDGER_PASSWORD);
+
+ // Add entries to ensure the right code path is validated
+ ledgerHandle.addEntry(TEST_LEDGER_ENTRY_DATA);
+ ledgerHandle.addEntry(TEST_LEDGER_ENTRY_DATA);
+ ledgerHandle.addEntry(TEST_LEDGER_ENTRY_DATA);
+
+ CheckerCallback cb = new CheckerCallback();
+ mockedChecker.checkLedger(ledgerHandle, cb);
+ Set<LedgerFragment> result = cb.waitAndGetResult();
+
+ // Note that the bookieWatcher mock is set to make the ledger
underreplicated
+ assertEquals("The one ledger should be considered underreplicated.",
1, result.size());
+ verify(bookieWatcher, times(3)).isBookieUnavailable(any());
+ }
+
private Set<LedgerFragment> getUnderReplicatedFragments(LedgerHandle lh)
throws InterruptedException {
LedgerChecker checker = new LedgerChecker(bkc);
diff --git
a/bookkeeper-server/src/test/java/org/apache/bookkeeper/replication/BookieAutoRecoveryTest.java
b/bookkeeper-server/src/test/java/org/apache/bookkeeper/replication/BookieAutoRecoveryTest.java
index 6362421..eb00dc1 100644
---
a/bookkeeper-server/src/test/java/org/apache/bookkeeper/replication/BookieAutoRecoveryTest.java
+++
b/bookkeeper-server/src/test/java/org/apache/bookkeeper/replication/BookieAutoRecoveryTest.java
@@ -627,12 +627,12 @@ public class BookieAutoRecoveryTest extends
BookKeeperClusterTestCase {
@Override
public void process(WatchedEvent event) {
if (event.getType() == EventType.NodeDeleted) {
- LOG.info("Recieved Ledger rereplication completion event :"
+ LOG.info("Received Ledger rereplication completion event :"
+ event.getType());
latch.countDown();
}
if (event.getType() == EventType.NodeCreated) {
- LOG.info("Recieved urLedger publishing event :"
+ LOG.info("Received urLedger publishing event :"
+ event.getType());
latch.countDown();
}
