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 2017/12/27 06:48:12 UTC
[GitHub] sijie closed pull request #876: Issue 870: ScanAndCompareGarbageCollector: harden against LM bugs
sijie closed pull request #876: Issue 870: ScanAndCompareGarbageCollector: harden against LM bugs
URL: https://github.com/apache/bookkeeper/pull/876
This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:
As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):
diff --git a/bookkeeper-server/conf/bk_server.conf b/bookkeeper-server/conf/bk_server.conf
index 7bdfa0d78..2fa4ea5d5 100755
--- a/bookkeeper-server/conf/bk_server.conf
+++ b/bookkeeper-server/conf/bk_server.conf
@@ -161,6 +161,9 @@ journalDirectory=/tmp/bk-txn
# log files are deleted after GC.
# isForceGCAllowWhenNoSpace=false
+# True if the bookie should double check readMetadata prior to gc
+# verifyMetadataOnGC=false
+
#############################################################################
## TLS settings
#############################################################################
diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/ScanAndCompareGarbageCollector.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/ScanAndCompareGarbageCollector.java
index 7399824a6..b42d302bb 100644
--- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/ScanAndCompareGarbageCollector.java
+++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/ScanAndCompareGarbageCollector.java
@@ -28,8 +28,10 @@
import java.util.NavigableSet;
import java.util.Set;
import java.util.SortedMap;
+import java.util.TreeSet;
import java.util.concurrent.CountDownLatch;
import java.util.concurrent.Semaphore;
+import java.util.concurrent.atomic.AtomicInteger;
import org.apache.bookkeeper.client.BKException;
import org.apache.bookkeeper.client.LedgerMetadata;
import org.apache.bookkeeper.conf.ServerConfiguration;
@@ -76,6 +78,7 @@
private final long gcOverReplicatedLedgerIntervalMillis;
private long lastOverReplicatedLedgerGcTimeMillis;
private final String zkLedgersRootPath;
+ private final boolean verifyMetadataOnGc;
public ScanAndCompareGarbageCollector(LedgerManager ledgerManager, CompactableLedgerStorage ledgerStorage,
ServerConfiguration conf) throws IOException {
@@ -91,6 +94,7 @@ public ScanAndCompareGarbageCollector(LedgerManager ledgerManager, CompactableLe
this.zkLedgersRootPath = conf.getZkLedgersRootPath();
LOG.info("Over Replicated Ledger Deletion : enabled=" + enableGcOverReplicatedLedger + ", interval="
+ gcOverReplicatedLedgerIntervalMillis);
+ verifyMetadataOnGc = conf.getVerifyMetadataOnGC();
}
@Override
@@ -106,18 +110,6 @@ public void gc(GarbageCleaner garbageCleaner) {
NavigableSet<Long> bkActiveLedgers = Sets.newTreeSet(ledgerStorage.getActiveLedgersInRange(0,
Long.MAX_VALUE));
- // Iterate over all the ledger on the metadata store
- LedgerRangeIterator ledgerRangeIterator = ledgerManager.getLedgerRanges();
-
- if (!ledgerRangeIterator.hasNext()) {
- // Empty global active ledgers, need to remove all local active ledgers.
- for (long ledgerId : bkActiveLedgers) {
- garbageCleaner.clean(ledgerId);
- }
- }
-
- long lastEnd = -1;
-
long curTime = MathUtils.now();
boolean checkOverreplicatedLedgers = (enableGcOverReplicatedLedger && curTime
- lastOverReplicatedLedgerGcTimeMillis > gcOverReplicatedLedgerIntervalMillis);
@@ -134,27 +126,50 @@ public void gc(GarbageCleaner garbageCleaner) {
lastOverReplicatedLedgerGcTimeMillis = MathUtils.now();
}
- while (ledgerRangeIterator.hasNext()) {
- LedgerRange lRange = ledgerRangeIterator.next();
-
- Long start = lastEnd + 1;
- Long end = lRange.end();
- if (!ledgerRangeIterator.hasNext()) {
+ // Iterate over all the ledger on the metadata store
+ LedgerRangeIterator ledgerRangeIterator = ledgerManager.getLedgerRanges();
+ Set<Long> ledgersInMetadata = null;
+ long start;
+ long end = -1;
+ boolean done = false;
+ while (!done) {
+ start = end + 1;
+ if (ledgerRangeIterator.hasNext()) {
+ LedgerRange lRange = ledgerRangeIterator.next();
+ ledgersInMetadata = lRange.getLedgers();
+ end = lRange.end();
+ } else {
+ ledgersInMetadata = new TreeSet<>();
end = Long.MAX_VALUE;
+ done = true;
}
Iterable<Long> subBkActiveLedgers = bkActiveLedgers.subSet(start, true, end, true);
- Set<Long> ledgersInMetadata = lRange.getLedgers();
if (LOG.isDebugEnabled()) {
LOG.debug("Active in metadata {}, Active in bookie {}", ledgersInMetadata, subBkActiveLedgers);
}
for (Long bkLid : subBkActiveLedgers) {
if (!ledgersInMetadata.contains(bkLid)) {
+ if (verifyMetadataOnGc) {
+ CountDownLatch latch = new CountDownLatch(1);
+ final AtomicInteger metaRC = new AtomicInteger(0);
+ ledgerManager.readLedgerMetadata(bkLid, (int rc, LedgerMetadata x) -> {
+ metaRC.set(rc);
+ latch.countDown();
+ });
+ latch.await();
+ if (metaRC.get() != BKException.Code.NoSuchLedgerExistsException) {
+ LOG.warn(
+ "Ledger {} Missing in metadata list, but ledgerManager returned rc: {}.",
+ bkLid,
+ metaRC.get());
+ continue;
+ }
+ }
garbageCleaner.clean(bkLid);
}
}
- lastEnd = end;
}
} catch (Throwable t) {
// ignore exception, collecting garbage next time
diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/ServerConfiguration.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/ServerConfiguration.java
index 38eccbae3..84d38ebbc 100644
--- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/ServerConfiguration.java
+++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/ServerConfiguration.java
@@ -53,6 +53,7 @@
protected static final String IS_FORCE_GC_ALLOW_WHEN_NO_SPACE = "isForceGCAllowWhenNoSpace";
protected static final String GC_OVERREPLICATED_LEDGER_WAIT_TIME = "gcOverreplicatedLedgerWaitTime";
protected static final String USE_TRANSACTIONAL_COMPACTION = "useTransactionalCompaction";
+ protected static final String VERIFY_METADATA_ON_GC = "verifyMetadataOnGC";
// Sync Parameters
protected static final String FLUSH_INTERVAL = "flushInterval";
protected static final String FLUSH_ENTRYLOG_INTERVAL_BYTES = "flushEntrylogBytes";
@@ -309,6 +310,25 @@ public ServerConfiguration setUseTransactionalCompaction(boolean useTransactiona
return this;
}
+ /**
+ * Get whether the bookie is configured to double check prior to gc.
+ *
+ * @return use transactional compaction
+ */
+ public boolean getVerifyMetadataOnGC() {
+ return this.getBoolean(VERIFY_METADATA_ON_GC, false);
+ }
+
+ /**
+ * Set whether the bookie is configured to double check prior to gc.
+ * @param verifyMetadataOnGC
+ * @return server configuration
+ */
+ public ServerConfiguration setVerifyMetadataOnGc(boolean verifyMetadataOnGC) {
+ this.setProperty(VERIFY_METADATA_ON_GC, verifyMetadataOnGC);
+ return this;
+ }
+
/**
* Get flush interval. Default value is 10 second. It isn't useful to decrease
* this value, since ledger storage only checkpoints when an entry logger file
diff --git a/bookkeeper-server/src/test/java/org/apache/bookkeeper/meta/GcLedgersTest.java b/bookkeeper-server/src/test/java/org/apache/bookkeeper/meta/GcLedgersTest.java
index 5c234d174..730490bae 100644
--- a/bookkeeper-server/src/test/java/org/apache/bookkeeper/meta/GcLedgersTest.java
+++ b/bookkeeper-server/src/test/java/org/apache/bookkeeper/meta/GcLedgersTest.java
@@ -43,6 +43,7 @@
import java.util.TreeSet;
import java.util.concurrent.CountDownLatch;
import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicBoolean;
import java.util.concurrent.atomic.AtomicInteger;
import org.apache.bookkeeper.bookie.BookieException;
import org.apache.bookkeeper.bookie.CheckpointSource;
@@ -139,7 +140,7 @@ public void operationComplete(int rc2, Void result) {
}
});
assertTrue(latch.await(10, TimeUnit.SECONDS));
- assertEquals("Remove should have succeeded", 0, rc.get());
+ assertEquals("Remove should have succeeded for ledgerId: " + ledgerId, 0, rc.get());
}
@Test
@@ -315,6 +316,229 @@ public void clean(long ledgerId) {
assertEquals("Should have cleaned first ledger" + first, (long) first, (long) cleaned.get(0));
}
+ /*
+ * in this scenario no ledger is created, so ledgeriterator's hasNext call would return false and next would be
+ * null. GarbageCollector.gc is expected to behave normally
+ */
+ @Test
+ public void testGcLedgersWithNoLedgers() throws Exception {
+ final SortedSet<Long> createdLedgers = Collections.synchronizedSortedSet(new TreeSet<Long>());
+ final List<Long> cleaned = new ArrayList<Long>();
+
+ // no ledger created
+
+ final GarbageCollector garbageCollector = new ScanAndCompareGarbageCollector(getLedgerManager(),
+ new MockLedgerStorage(), baseConf);
+ AtomicBoolean cleanerCalled = new AtomicBoolean(false);
+
+ GarbageCollector.GarbageCleaner cleaner = new GarbageCollector.GarbageCleaner() {
+ @Override
+ public void clean(long ledgerId) {
+ LOG.info("Cleaned {}", ledgerId);
+ cleanerCalled.set(true);
+ }
+ };
+
+ validateLedgerRangeIterator(createdLedgers);
+
+ garbageCollector.gc(cleaner);
+ assertFalse("Should have cleaned nothing, since no ledger is created", cleanerCalled.get());
+ }
+
+ // in this scenario all the created ledgers are in one single ledger range.
+ @Test
+ public void testGcLedgersWithLedgersInSameLedgerRange() throws Exception {
+ baseConf.setVerifyMetadataOnGc(true);
+ final SortedSet<Long> createdLedgers = Collections.synchronizedSortedSet(new TreeSet<Long>());
+ final SortedSet<Long> cleaned = Collections.synchronizedSortedSet(new TreeSet<Long>());
+
+ // Create few ledgers which span over just one ledger range in the hierarchical ledger manager implementation
+ final int numLedgers = 5;
+
+ createLedgers(numLedgers, createdLedgers);
+
+ final GarbageCollector garbageCollector = new ScanAndCompareGarbageCollector(getLedgerManager(),
+ new MockLedgerStorage(), baseConf);
+ GarbageCollector.GarbageCleaner cleaner = new GarbageCollector.GarbageCleaner() {
+ @Override
+ public void clean(long ledgerId) {
+ LOG.info("Cleaned {}", ledgerId);
+ cleaned.add(ledgerId);
+ }
+ };
+
+ validateLedgerRangeIterator(createdLedgers);
+
+ garbageCollector.gc(cleaner);
+ assertTrue("Should have cleaned nothing", cleaned.isEmpty());
+
+ for (long ledgerId : createdLedgers) {
+ removeLedger(ledgerId);
+ }
+
+ garbageCollector.gc(cleaner);
+ assertEquals("Should have cleaned all the created ledgers", createdLedgers, cleaned);
+ }
+
+ /*
+ * in this test scenario no created ledger is deleted, but ledgeriterator is screwed up and returns hasNext to be
+ * false and next to be null. So even in this case it is expected not to clean any ledger's data.
+ *
+ * This testcase is needed for validating fix of bug - W-4292747.
+ *
+ * ScanAndCompareGarbageCollector/GC should clean data of ledger only if both the LedgerManager.getLedgerRanges says
+ * that ledger is not existing and also ledgerManager.readLedgerMetadata fails with error
+ * NoSuchLedgerExistsException.
+ *
+ */
+ @Test
+ public void testGcLedgersIfLedgerManagerIteratorFails() throws Exception {
+ baseConf.setVerifyMetadataOnGc(true);
+ final SortedSet<Long> createdLedgers = Collections.synchronizedSortedSet(new TreeSet<Long>());
+ final SortedSet<Long> cleaned = Collections.synchronizedSortedSet(new TreeSet<Long>());
+
+ // Create few ledgers
+ final int numLedgers = 5;
+
+ createLedgers(numLedgers, createdLedgers);
+
+ LedgerManager mockLedgerManager = new CleanupLedgerManager(getLedgerManager()) {
+ @Override
+ public LedgerRangeIterator getLedgerRanges() {
+ return new LedgerRangeIterator() {
+ @Override
+ public LedgerRange next() throws IOException {
+ return null;
+ }
+
+ @Override
+ public boolean hasNext() throws IOException {
+ return false;
+ }
+ };
+ }
+ };
+
+ final GarbageCollector garbageCollector = new ScanAndCompareGarbageCollector(mockLedgerManager,
+ new MockLedgerStorage(), baseConf);
+ GarbageCollector.GarbageCleaner cleaner = new GarbageCollector.GarbageCleaner() {
+ @Override
+ public void clean(long ledgerId) {
+ LOG.info("Cleaned {}", ledgerId);
+ cleaned.add(ledgerId);
+ }
+ };
+
+ validateLedgerRangeIterator(createdLedgers);
+
+ garbageCollector.gc(cleaner);
+ assertTrue("Should have cleaned nothing", cleaned.isEmpty());
+ }
+
+ /*
+ * In this test scenario no ledger is deleted, but LedgerManager.readLedgerMetadata says there is NoSuchLedger. So
+ * even in that case, GarbageCollector.gc shouldn't delete ledgers data.
+ *
+ * Consider the possible scenario - when the LedgerIterator is created that ledger is not deleted, so as per
+ * LedgerIterator that is live ledger. But right after the LedgerIterator creation that ledger is deleted, so
+ * readLedgerMetadata call would return NoSuchLedger. In this testscenario we are validating that as per Iterator if
+ * that ledger is alive though currently that ledger is deleted, we should not clean data of that ledger.
+ *
+ * ScanAndCompareGarbageCollector/GC should clean data of ledger only if both the LedgerManager.getLedgerRanges says
+ * that ledger is not existing and also ledgerManager.readLedgerMetadata fails with error
+ * NoSuchLedgerExistsException.
+ *
+ */
+ @Test
+ public void testGcLedgersIfReadLedgerMetadataSaysNoSuchLedger() throws Exception {
+ final SortedSet<Long> createdLedgers = Collections.synchronizedSortedSet(new TreeSet<Long>());
+ final SortedSet<Long> cleaned = Collections.synchronizedSortedSet(new TreeSet<Long>());
+
+ // Create few ledgers
+ final int numLedgers = 5;
+
+ createLedgers(numLedgers, createdLedgers);
+
+ LedgerManager mockLedgerManager = new CleanupLedgerManager(getLedgerManager()) {
+ @Override
+ public void readLedgerMetadata(long ledgerId, GenericCallback<LedgerMetadata> readCb) {
+ readCb.operationComplete(BKException.Code.NoSuchLedgerExistsException, null);
+ }
+ };
+
+ final GarbageCollector garbageCollector = new ScanAndCompareGarbageCollector(mockLedgerManager,
+ new MockLedgerStorage(), baseConf);
+ GarbageCollector.GarbageCleaner cleaner = new GarbageCollector.GarbageCleaner() {
+ @Override
+ public void clean(long ledgerId) {
+ LOG.info("Cleaned {}", ledgerId);
+ cleaned.add(ledgerId);
+ }
+ };
+
+ validateLedgerRangeIterator(createdLedgers);
+
+ garbageCollector.gc(cleaner);
+ assertTrue("Should have cleaned nothing", cleaned.isEmpty());
+ }
+
+ /*
+ * In this test scenario all the created ledgers are deleted, but LedgerManager.readLedgerMetadata fails with
+ * ZKException. So even in this case, GarbageCollector.gc shouldn't delete ledgers data.
+ *
+ * ScanAndCompareGarbageCollector/GC should clean data of ledger only if both the LedgerManager.getLedgerRanges says
+ * that ledger is not existing and also ledgerManager.readLedgerMetadata fails with error
+ * NoSuchLedgerExistsException, but is shouldn't delete if the readLedgerMetadata fails with any other error.
+ */
+ @Test
+ public void testGcLedgersIfReadLedgerMetadataFailsForDeletedLedgers() throws Exception {
+ baseConf.setVerifyMetadataOnGc(true);
+ final SortedSet<Long> createdLedgers = Collections.synchronizedSortedSet(new TreeSet<Long>());
+ final SortedSet<Long> cleaned = Collections.synchronizedSortedSet(new TreeSet<Long>());
+
+ // Create few ledgers
+ final int numLedgers = 5;
+
+ createLedgers(numLedgers, createdLedgers);
+
+ LedgerManager mockLedgerManager = new CleanupLedgerManager(getLedgerManager()) {
+ @Override
+ public void readLedgerMetadata(long ledgerId, GenericCallback<LedgerMetadata> readCb) {
+ readCb.operationComplete(BKException.Code.ZKException, null);
+ }
+ };
+
+ final GarbageCollector garbageCollector = new ScanAndCompareGarbageCollector(mockLedgerManager,
+ new MockLedgerStorage(), baseConf);
+ GarbageCollector.GarbageCleaner cleaner = new GarbageCollector.GarbageCleaner() {
+ @Override
+ public void clean(long ledgerId) {
+ LOG.info("Cleaned {}", ledgerId);
+ cleaned.add(ledgerId);
+ }
+ };
+
+ validateLedgerRangeIterator(createdLedgers);
+
+ for (long ledgerId : createdLedgers) {
+ removeLedger(ledgerId);
+ }
+
+ garbageCollector.gc(cleaner);
+ assertTrue("Should have cleaned nothing", cleaned.isEmpty());
+ }
+
+ public void validateLedgerRangeIterator(SortedSet<Long> createdLedgers) throws IOException {
+ SortedSet<Long> scannedLedgers = new TreeSet<Long>();
+ LedgerRangeIterator iterator = getLedgerManager().getLedgerRanges();
+ while (iterator.hasNext()) {
+ LedgerRange ledgerRange = iterator.next();
+ scannedLedgers.addAll(ledgerRange.getLedgers());
+ }
+
+ assertEquals(createdLedgers, scannedLedgers);
+ }
+
class MockLedgerStorage implements CompactableLedgerStorage {
@Override
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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