Skip to content

Commit

Permalink
ISSUE #870: ScanAndCompareGarbageCollector: harden against LM bugs
Browse files Browse the repository at this point in the history 
The idea behind this patch is to make it more likely that more than one
LedgerManager bug would be required to erroneously believe a ledger to be
deleted.
1) Remove the special handling for the completely empty LM.  It's not
really a common case nor is it much of an optimization even when it
happens.
2) Add a config variable to cause the bookie to also check the
LedgerManager.readLedgerMetadata path before actually removing a ledger.
The implementations of readLedgerMetadata and the iterator are
sufficiently different that it seems worth it to have the option of
checking both to guard somewhat against a bug in either.

(@bug W-4292747@)
(@bug W-3027938@)
Signed-off-by: Samuel Just <sjust@salesforce.com>
Signed-off-by: Charan Reddy Guttapalem <cguttapalem@salesforce.com>
  • Loading branch information
Samuel Just committed Dec 20, 2017
1 parent 97c35e9 commit 65a83f5
Show file tree
Hide file tree
Showing 4 changed files with 281 additions and 21 deletions.
3 changes: 3 additions & 0 deletions bookkeeper-server/conf/bk_server.conf
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
#############################################################################
Expand Down
53 changes: 33 additions & 20 deletions ...per-server/src/main/java/org/apache/bookkeeper/bookie/ScanAndCompareGarbageCollector.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -106,18 +108,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);
Expand All @@ -134,27 +124,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 (conf.getVerifyMetadataOnGC()) {
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
Expand Down
20 changes: 20 additions & 0 deletions bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/ServerConfiguration.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ public class ServerConfiguration extends AbstractConfiguration<ServerConfigurati
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";
Expand Down Expand Up @@ -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
Expand Down
226 changes: 225 additions & 1 deletion bookkeeper-server/src/test/java/org/apache/bookkeeper/meta/GcLedgersTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down

0 comments on commit 65a83f5

Please sign in to comment.