Skip to content
Permalink
Browse files
[BK-GC] avoid blocking call in gc-thread
### Motivation

Right now, we have below 3 issues because of which gc thread gets blocked forever and it can't perform gc-task further. Below issues are mainly related to blocking call while doing zk-operation without timeout. 

bug-fixes:
1. right now, [GC - ScanAndCompareGarbageCollector](https://github.com/apache/bookkeeper/blob/master/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/ScanAndCompareGarbageCollector.java#L142) passes timeout in millisecond to [LedgerManager](https://github.com/apache/bookkeeper/blob/master/bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/LongHierarchicalLedgerManager.java#L166) but it 
takes it as second and again try to convert it in millis so, 30Kms timeout becomes [30M ms timeout](https://github.com/apache/bookkeeper/blob/master/bookkeeper-server/src/main/java/org/apache/bookkeeper/util/ZkUtils.java#L245). Sp, fix timeout unit during gc.

2. Right now, GC makes blocking call to get list of children on ledger znode and sometime zk-call back doesn't comeback which blocks the gc-thread forever. However, recently we added the timeout on the [object-waiting-lock](https://github.com/apache/bookkeeper/blob/master/bookkeeper-server/src/main/java/org/apache/bookkeeper/util/ZkUtils.java#L243-L248) which doesn't work because it's in while loop and `object.wait(timeout)` completes without any exception and GC threads keep running in while loop.

3. add zk-timeout during delete ledgers in bookie else it can also block the GC thread.





### Changes

add timeout while bk-gc makes zk-call to verify deleted ledgers.

Reviewers: Enrico Olivelli <eolivelli@gmail.com>, Sijie Guo <sijie@apache.org>, Rajan Dhabalia <rdhabalia@apache.org>

This closes #1940 from rdhabalia/verify_gc
  • Loading branch information
rdhabalia authored and eolivelli committed May 15, 2019
1 parent d35aa22 commit f5ddc36cc30c5bab5d2e8ebc5fa552c2ad0d6eea
Showing 13 changed files with 53 additions and 36 deletions.
@@ -32,6 +32,9 @@
import java.util.TreeSet;
import java.util.concurrent.CountDownLatch;
import java.util.concurrent.Semaphore;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.TimeoutException;

import org.apache.bookkeeper.client.BKException;
import org.apache.bookkeeper.conf.ServerConfiguration;
import org.apache.bookkeeper.meta.LedgerManager;
@@ -139,8 +142,9 @@ public void gc(GarbageCleaner garbageCleaner) {
}

// Iterate over all the ledger on the metadata store
long zkOpTimeout = this.conf.getZkTimeout() * 2;
LedgerRangeIterator ledgerRangeIterator = ledgerManager.getLedgerRanges(zkOpTimeout);
long zkOpTimeoutMs = this.conf.getZkTimeout() * 2;
LedgerRangeIterator ledgerRangeIterator = ledgerManager
.getLedgerRanges(zkOpTimeoutMs);
Set<Long> ledgersInMetadata = null;
long start;
long end = -1;
@@ -167,13 +171,20 @@ public void gc(GarbageCleaner garbageCleaner) {
if (verifyMetadataOnGc) {
int rc = BKException.Code.OK;
try {
result(ledgerManager.readLedgerMetadata(bkLid));
} catch (BKException e) {
rc = e.getCode();
result(ledgerManager.readLedgerMetadata(bkLid), zkOpTimeoutMs, TimeUnit.MILLISECONDS);
} catch (BKException | TimeoutException e) {
if (e instanceof BKException) {
rc = ((BKException) e).getCode();
} else {
LOG.warn("Time-out while fetching metadata for Ledger {} : {}.", bkLid,
e.getMessage());

continue;
}
}
if (rc != BKException.Code.NoSuchLedgerExistsOnMetadataServerException) {
LOG.warn("Ledger {} Missing in metadata list, but ledgerManager returned rc: {}.",
bkLid, rc);
bkLid, rc);
continue;
}
}
@@ -210,13 +210,13 @@ public void processResult(int rc, String path, Object ctx) {
}

@Override
public LedgerRangeIterator getLedgerRanges(long zkOpTimeoutSec) {
public LedgerRangeIterator getLedgerRanges(long zkOpTimeoutMs) {
closeLock.readLock().lock();
try {
if (closed) {
return new ClosedLedgerRangeIterator();
}
return underlying.getLedgerRanges(zkOpTimeoutSec);
return underlying.getLedgerRanges(zkOpTimeoutMs);
} finally {
closeLock.readLock().unlock();
}
@@ -89,7 +89,7 @@ public void asyncProcessLedgers(final Processor<Long> processor,
}

@Override
public LedgerRangeIterator getLedgerRanges(long zkOpTimeOutSec) {
public LedgerRangeIterator getLedgerRanges(long zkOpTimeoutMs) {
return new LedgerRangeIterator() {
// single iterator, can visit only one time
boolean nextCalled = false;
@@ -103,7 +103,7 @@ private synchronized void preload() throws IOException {

try {
zkActiveLedgers = ledgerListToSet(
ZkUtils.getChildrenInSingleNode(zk, ledgerRootPath, zkOpTimeOutSec),
ZkUtils.getChildrenInSingleNode(zk, ledgerRootPath, zkOpTimeoutMs),
ledgerRootPath);
nextRange = new LedgerRange(zkActiveLedgers);
} catch (KeeperException.NoNodeException e) {
@@ -87,9 +87,9 @@ protected long getLedgerId(String ledgerPath) throws IOException {
}

@Override
public LedgerRangeIterator getLedgerRanges(long zkOpTimeoutSec) {
LedgerRangeIterator legacyLedgerRangeIterator = legacyLM.getLedgerRanges(zkOpTimeoutSec);
LedgerRangeIterator longLedgerRangeIterator = longLM.getLedgerRanges(zkOpTimeoutSec);
public LedgerRangeIterator getLedgerRanges(long zkOpTimeoutMs) {
LedgerRangeIterator legacyLedgerRangeIterator = legacyLM.getLedgerRanges(zkOpTimeoutMs);
LedgerRangeIterator longLedgerRangeIterator = longLM.getLedgerRanges(zkOpTimeoutMs);
return new HierarchicalLedgerRangeIterator(legacyLedgerRangeIterator, longLedgerRangeIterator);
}

@@ -149,12 +149,12 @@ void asyncProcessLedgers(Processor<Long> processor, AsyncCallback.VoidCallback f
/**
* Loop to scan a range of metadata from metadata storage.
*
* @param zkOpTimeOutSec
* @param zkOpTimeOutMs
* Iterator considers timeout while fetching ledger-range from
* zk.
* @return will return a iterator of the Ranges
*/
LedgerRangeIterator getLedgerRanges(long zkOpTimeOutSec);
LedgerRangeIterator getLedgerRanges(long zkOpTimeOutMs);

/**
* Used to represent the Ledgers range returned from the
@@ -153,8 +153,8 @@ protected String getLedgerParentNodeRegex() {
}

@Override
public LedgerRangeIterator getLedgerRanges(long zkOpTimeoutSec) {
return new LegacyHierarchicalLedgerRangeIterator(zkOpTimeoutSec);
public LedgerRangeIterator getLedgerRanges(long zkOpTimeoutMs) {
return new LegacyHierarchicalLedgerRangeIterator(zkOpTimeoutMs);
}

/**
@@ -166,10 +166,10 @@ private class LegacyHierarchicalLedgerRangeIterator implements LedgerRangeIterat
private String curL1Nodes = "";
private boolean iteratorDone = false;
private LedgerRange nextRange = null;
private final long zkOpTimeoutSec;
private final long zkOpTimeoutMs;

public LegacyHierarchicalLedgerRangeIterator(long zkOpTimeoutSec) {
this.zkOpTimeoutSec = zkOpTimeoutSec;
public LegacyHierarchicalLedgerRangeIterator(long zkOpTimeoutMs) {
this.zkOpTimeoutMs = zkOpTimeoutMs;
}

/**
@@ -266,7 +266,7 @@ LedgerRange getLedgerRangeByLevel(final String level1, final String level2)
String nodePath = nodeBuilder.toString();
List<String> ledgerNodes = null;
try {
ledgerNodes = ZkUtils.getChildrenInSingleNode(zk, nodePath, zkOpTimeoutSec);
ledgerNodes = ZkUtils.getChildrenInSingleNode(zk, nodePath, zkOpTimeoutMs);
} catch (KeeperException.NoNodeException e) {
/* If the node doesn't exist, we must have raced with a recursive node removal, just
* return an empty list. */
@@ -139,8 +139,8 @@ public void process(String lNode, VoidCallback cb) {
}

@Override
public LedgerRangeIterator getLedgerRanges(long zkOpTimeoutSec) {
return new LongHierarchicalLedgerRangeIterator(zkOpTimeoutSec);
public LedgerRangeIterator getLedgerRanges(long zkOpTimeoutMs) {
return new LongHierarchicalLedgerRangeIterator(zkOpTimeoutMs);
}


@@ -149,7 +149,7 @@ public LedgerRangeIterator getLedgerRanges(long zkOpTimeoutSec) {
*/
private class LongHierarchicalLedgerRangeIterator implements LedgerRangeIterator {
LedgerRangeIterator rootIterator;
final long zkOpTimeoutSec;
final long zkOpTimeoutMs;

/**
* Returns all children with path as a parent. If path is non-existent,
@@ -163,7 +163,7 @@ private class LongHierarchicalLedgerRangeIterator implements LedgerRangeIterator
*/
List<String> getChildrenAt(String path) throws IOException {
try {
List<String> children = ZkUtils.getChildrenInSingleNode(zk, path, zkOpTimeoutSec);
List<String> children = ZkUtils.getChildrenInSingleNode(zk, path, zkOpTimeoutMs);
Collections.sort(children);
return children;
} catch (KeeperException.NoNodeException e) {
@@ -285,8 +285,8 @@ public LedgerRange next() throws IOException {
}
}

private LongHierarchicalLedgerRangeIterator(long zkOpTimeoutSec) {
this.zkOpTimeoutSec = zkOpTimeoutSec;
private LongHierarchicalLedgerRangeIterator(long zkOpTimeoutMs) {
this.zkOpTimeoutMs = zkOpTimeoutMs;
}

private void bootstrap() throws IOException {
@@ -642,7 +642,7 @@ public LedgerRange next() throws IOException {
}

@Override
public LedgerRangeIterator getLedgerRanges(long zkOpTimeoutSec) {
public LedgerRangeIterator getLedgerRanges(long zkOpTimeoutMs) {
return new MSLedgerRangeIterator();
}

@@ -25,7 +25,6 @@
import java.io.IOException;
import java.util.List;
import java.util.concurrent.CountDownLatch;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicInteger;

import org.apache.bookkeeper.conf.AbstractConfiguration;
@@ -222,7 +221,7 @@ private static class GetChildrenCtx {
* @throws InterruptedException
* @throws IOException
*/
public static List<String> getChildrenInSingleNode(final ZooKeeper zk, final String node, long timeOutSec)
public static List<String> getChildrenInSingleNode(final ZooKeeper zk, final String node, long zkOpTimeoutMs)
throws InterruptedException, IOException, KeeperException.NoNodeException {
final GetChildrenCtx ctx = new GetChildrenCtx();
getChildrenInSingleNode(zk, node, new GenericCallback<List<String>>() {
@@ -240,13 +239,20 @@ public void operationComplete(int rc, List<String> ledgers) {
});

synchronized (ctx) {
long startTime = System.currentTimeMillis();
while (!ctx.done) {
try {
ctx.wait(timeOutSec > 0 ? TimeUnit.SECONDS.toMillis(timeOutSec) : 0);
ctx.wait(zkOpTimeoutMs > 0 ? zkOpTimeoutMs : 0);
} catch (InterruptedException e) {
ctx.rc = Code.OPERATIONTIMEOUT.intValue();
ctx.done = true;
}
// timeout the process if get-children response not received
// zkOpTimeoutMs.
if (zkOpTimeoutMs > 0 && (System.currentTimeMillis() - startTime) >= zkOpTimeoutMs) {
ctx.rc = Code.OPERATIONTIMEOUT.intValue();
ctx.done = true;
}
}
}
if (Code.NONODE.intValue() == ctx.rc) {
@@ -970,7 +970,7 @@ void unsupported() {
}

@Override
public LedgerRangeIterator getLedgerRanges(long zkOpTimeoutSec) {
public LedgerRangeIterator getLedgerRanges(long zkOpTimeoutMs) {
final AtomicBoolean hasnext = new AtomicBoolean(true);
return new LedgerManager.LedgerRangeIterator() {
@Override
@@ -112,8 +112,8 @@ public CompletableFuture<Versioned<LedgerMetadata>> readLedgerMetadata(long ledg
}

@Override
public LedgerRangeIterator getLedgerRanges(long zkOpTimeoutSec) {
return lm.getLedgerRanges(zkOpTimeoutSec);
public LedgerRangeIterator getLedgerRanges(long zkOpTimeoutMs) {
return lm.getLedgerRanges(zkOpTimeoutMs);
}

@Override
@@ -188,7 +188,7 @@ public void asyncProcessLedgers(Processor<Long> processor, AsyncCallback.VoidCal
}

@Override
public LedgerRangeIterator getLedgerRanges(long zkOpTimeoutSec) {
public LedgerRangeIterator getLedgerRanges(long zkOpTimeoutMs) {
return null;
}

@@ -420,7 +420,7 @@ private void processLedgers(KeyStream<Long> ks,
}

@Override
public LedgerRangeIterator getLedgerRanges(long opTimeOutSec) {
public LedgerRangeIterator getLedgerRanges(long opTimeOutMs) {
KeyStream<Long> ks = new KeyStream<>(
kvClient,
ByteSequence.fromString(EtcdUtils.getLedgerKey(scope, 0L)),

0 comments on commit f5ddc36

Please sign in to comment.