diff --git a/bookkeeper-server/conf/bk_server.conf b/bookkeeper-server/conf/bk_server.conf index 7bdfa0d7896..2fa4ea5d59b 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 7399824a6e9..b42d302bb36 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 @@ public class ScanAndCompareGarbageCollector implements GarbageCollector{ 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 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 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 subBkActiveLedgers = bkActiveLedgers.subSet(start, true, end, true); - Set 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 38eccbae382..84d38ebbc36 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 @@ public class ServerConfiguration extends AbstractConfiguration createdLedgers = Collections.synchronizedSortedSet(new TreeSet()); + final List cleaned = new ArrayList(); + + // 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 createdLedgers = Collections.synchronizedSortedSet(new TreeSet()); + final SortedSet cleaned = Collections.synchronizedSortedSet(new TreeSet()); + + // 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 createdLedgers = Collections.synchronizedSortedSet(new TreeSet()); + final SortedSet cleaned = Collections.synchronizedSortedSet(new TreeSet()); + + // 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 createdLedgers = Collections.synchronizedSortedSet(new TreeSet()); + final SortedSet cleaned = Collections.synchronizedSortedSet(new TreeSet()); + + // Create few ledgers + final int numLedgers = 5; + + createLedgers(numLedgers, createdLedgers); + + LedgerManager mockLedgerManager = new CleanupLedgerManager(getLedgerManager()) { + @Override + public void readLedgerMetadata(long ledgerId, GenericCallback 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 createdLedgers = Collections.synchronizedSortedSet(new TreeSet()); + final SortedSet cleaned = Collections.synchronizedSortedSet(new TreeSet()); + + // Create few ledgers + final int numLedgers = 5; + + createLedgers(numLedgers, createdLedgers); + + LedgerManager mockLedgerManager = new CleanupLedgerManager(getLedgerManager()) { + @Override + public void readLedgerMetadata(long ledgerId, GenericCallback 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 createdLedgers) throws IOException { + SortedSet scannedLedgers = new TreeSet(); + LedgerRangeIterator iterator = getLedgerManager().getLedgerRanges(); + while (iterator.hasNext()) { + LedgerRange ledgerRange = iterator.next(); + scannedLedgers.addAll(ledgerRange.getLedgers()); + } + + assertEquals(createdLedgers, scannedLedgers); + } + class MockLedgerStorage implements CompactableLedgerStorage { @Override