Author: ivank Date: Thu Apr 4 09:21:26 2013 New Revision: 1464385 URL: http://svn.apache.org/r1464385 Log: BOOKKEEPER-596: Ledgers are gc'ed by mistake in MSLedgerManagerFactory. (sijie & ivank) Modified: zookeeper/bookkeeper/trunk/CHANGES.txt zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/ScanAndCompareGarbageCollector.java zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/AbstractZkLedgerManager.java zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/FlatLedgerManager.java zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/HierarchicalLedgerManager.java zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/LedgerManager.java zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/MSLedgerManagerFactory.java zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/metastore/InMemoryMetastoreCursor.java zookeeper/bookkeeper/trunk/bookkeeper-server/src/test/java/org/apache/bookkeeper/meta/GcLedgersTest.java zookeeper/bookkeeper/trunk/bookkeeper-server/src/test/java/org/apache/bookkeeper/meta/LedgerManagerTestCase.java Modified: zookeeper/bookkeeper/trunk/CHANGES.txt URL: http://svn.apache.org/viewvc/zookeeper/bookkeeper/trunk/CHANGES.txt?rev=1464385&r1=1464384&r2=1464385&view=diff ============================================================================== --- zookeeper/bookkeeper/trunk/CHANGES.txt (original) +++ zookeeper/bookkeeper/trunk/CHANGES.txt Thu Apr 4 09:21:26 2013 @@ -10,6 +10,8 @@ Trunk (unreleased changes) BUGFIXES: + BOOKKEEPER-596: Ledgers are gc'ed by mistake in MSLedgerManagerFactory. (sijie & ivank) + BOOKKEEPER-595: Crash of inprocess autorecovery daemon should not take down the bookie (ivank) BOOKKEEPER-597: Add flag to output test logs to stdout (ivank) Modified: zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/ScanAndCompareGarbageCollector.java URL: http://svn.apache.org/viewvc/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/ScanAndCompareGarbageCollector.java?rev=1464385&r1=1464384&r2=1464385&view=diff ============================================================================== --- zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/ScanAndCompareGarbageCollector.java (original) +++ zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/ScanAndCompareGarbageCollector.java Thu Apr 4 09:21:26 2013 @@ -72,31 +72,31 @@ public class ScanAndCompareGarbageCollec garbageCleaner.clean(bkLid); } } + long lastEnd = -1; + while(ledgerRangeIterator.hasNext()) { LedgerRange lRange = ledgerRangeIterator.next(); Map subBkActiveLedgers = null; - Long start = lRange.start(); + + Long start = lastEnd + 1; Long end = lRange.end(); - if (end != LedgerRange.NOLIMIT) { - subBkActiveLedgers = bkActiveLedgersSnapshot.subMap(start, - true, end, true); - } else { - if (start != LedgerRange.NOLIMIT) { - subBkActiveLedgers = bkActiveLedgersSnapshot.tailMap(start); - } else { - subBkActiveLedgers = bkActiveLedgersSnapshot; - } + if (!ledgerRangeIterator.hasNext()) { + end = Long.MAX_VALUE; } - Set globalActiveLedgers = lRange.getLedgers(); - LOG.debug("All active ledgers for hash node {}, Current active ledgers from Bookie for hash node {}", - globalActiveLedgers, subBkActiveLedgers.keySet()); + subBkActiveLedgers = bkActiveLedgersSnapshot.subMap( + start, true, end, true); + + Set ledgersInMetadata = lRange.getLedgers(); + LOG.debug("Active in metadata {}, Active in bookie {}", + ledgersInMetadata, subBkActiveLedgers.keySet()); for (Long bkLid : subBkActiveLedgers.keySet()) { - if (!globalActiveLedgers.contains(bkLid)) { + if (!ledgersInMetadata.contains(bkLid)) { // remove it from current active ledger subBkActiveLedgers.remove(bkLid); garbageCleaner.clean(bkLid); } } + lastEnd = end; } } catch (Exception e) { // ignore exception, collecting garbage next time Modified: zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/AbstractZkLedgerManager.java URL: http://svn.apache.org/viewvc/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/AbstractZkLedgerManager.java?rev=1464385&r1=1464384&r2=1464385&view=diff ============================================================================== --- zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/AbstractZkLedgerManager.java (original) +++ zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/AbstractZkLedgerManager.java Thu Apr 4 09:21:26 2013 @@ -18,7 +18,8 @@ package org.apache.bookkeeper.meta; import java.io.IOException; -import java.util.HashSet; +import java.util.TreeSet; +import java.util.SortedSet; import java.util.List; import java.util.Set; @@ -275,8 +276,8 @@ abstract class AbstractZkLedgerManager i * the prefix path of the ledger nodes * @return ledger id hash set */ - protected Set ledgerListToSet(List ledgerNodes, String path) { - Set zkActiveLedgers = new HashSet(ledgerNodes.size(), 1.0f); + protected SortedSet ledgerListToSet(List ledgerNodes, String path) { + SortedSet zkActiveLedgers = new TreeSet(); for (String ledgerNode : ledgerNodes) { if (isSpecialZnode(ledgerNode)) { continue; Modified: zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/FlatLedgerManager.java URL: http://svn.apache.org/viewvc/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/FlatLedgerManager.java?rev=1464385&r1=1464384&r2=1464385&view=diff ============================================================================== --- zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/FlatLedgerManager.java (original) +++ zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/FlatLedgerManager.java Thu Apr 4 09:21:26 2013 @@ -128,25 +128,38 @@ class FlatLedgerManager extends Abstract public LedgerRangeIterator getLedgerRanges() { return new LedgerRangeIterator() { // single iterator, can visit only one time - boolean hasMoreElement = true; - @Override - public boolean hasNext() { - return hasMoreElement; - } - @Override - public LedgerRange next() throws IOException { - if (!hasMoreElement) { - throw new NoSuchElementException(); + boolean nextCalled = false; + LedgerRange nextRange = null; + + synchronized private void preload() throws IOException { + if (nextRange != null) { + return; } - hasMoreElement = false; - Set zkActiveLedgers; + Set zkActiveLedgers = null; + try { zkActiveLedgers = ledgerListToSet( ZkUtils.getChildrenInSingleNode(zk, ledgerRootPath), ledgerRootPath); - } catch (InterruptedException e) { - throw new IOException("Error when get child nodes from zk", e); + nextRange = new LedgerRange(zkActiveLedgers); + } catch (InterruptedException ie) { + Thread.currentThread().interrupt(); + throw new IOException("Error when get child nodes from zk", ie); + } + } + + @Override + synchronized public boolean hasNext() throws IOException { + preload(); + return nextRange != null && nextRange.size() > 0 && !nextCalled; + } + + @Override + synchronized public LedgerRange next() throws IOException { + if (!hasNext()) { + throw new NoSuchElementException(); } - return new LedgerRange(zkActiveLedgers); + nextCalled = true; + return nextRange; } }; } Modified: zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/HierarchicalLedgerManager.java URL: http://svn.apache.org/viewvc/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/HierarchicalLedgerManager.java?rev=1464385&r1=1464384&r2=1464385&view=diff ============================================================================== --- zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/HierarchicalLedgerManager.java (original) +++ zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/HierarchicalLedgerManager.java Thu Apr 4 09:21:26 2013 @@ -25,7 +25,7 @@ import java.util.concurrent.atomic.Atomi import java.util.Iterator; import java.util.List; import java.util.NoSuchElementException; -import java.util.Set; +import java.util.SortedSet; import org.apache.bookkeeper.client.LedgerMetadata; import org.apache.bookkeeper.conf.AbstractConfiguration; @@ -381,7 +381,8 @@ class HierarchicalLedgerManager extends private Iterator l1NodesIter = null; private Iterator l2NodesIter = null; private String curL1Nodes = ""; - private boolean hasMoreElement = true; + private boolean iteratorDone = false; + private LedgerRange nextRange = null; /** * iterate next level1 znode @@ -410,27 +411,47 @@ class HierarchicalLedgerManager extends return true; } - @Override - public boolean hasNext() throws IOException { - try { - if (l1NodesIter == null) { - l1NodesIter = zk.getChildren(ledgerRootPath, null).iterator(); - hasMoreElement = nextL1Node(); - } else if (l2NodesIter == null || !l2NodesIter.hasNext()) { - hasMoreElement = nextL1Node(); + synchronized private void preload() throws IOException { + while (nextRange == null && !iteratorDone) { + boolean hasMoreElements = false; + try { + if (l1NodesIter == null) { + l1NodesIter = zk.getChildren(ledgerRootPath, null).iterator(); + hasMoreElements = nextL1Node(); + } else if (l2NodesIter == null || !l2NodesIter.hasNext()) { + hasMoreElements = nextL1Node(); + } + } catch (KeeperException ke) { + throw new IOException("Error preloading next range", ke); + } catch (InterruptedException ie) { + Thread.currentThread().interrupt(); + throw new IOException("Interrupted while preloading", ie); + } + if (hasMoreElements) { + nextRange = getLedgerRangeByLevel(curL1Nodes, l2NodesIter.next()); + if (nextRange.size() == 0) { + nextRange = null; + } + } else { + iteratorDone = true; } - } catch (Exception e) { - throw new IOException("Error when check more elements", e); } - return hasMoreElement; } @Override - public LedgerRange next() throws IOException { - if (!hasMoreElement) { + synchronized public boolean hasNext() throws IOException { + preload(); + return nextRange != null && !iteratorDone; + } + + @Override + synchronized public LedgerRange next() throws IOException { + if (!hasNext()) { throw new NoSuchElementException(); } - return getLedgerRangeByLevel(curL1Nodes, l2NodesIter.next()); + LedgerRange r = nextRange; + nextRange = null; + return r; } /** @@ -454,13 +475,13 @@ class HierarchicalLedgerManager extends } catch (InterruptedException e) { throw new IOException("Error when get child nodes from zk", e); } - Set zkActiveLedgers = ledgerListToSet(ledgerNodes, nodePath); + SortedSet zkActiveLedgers = ledgerListToSet(ledgerNodes, nodePath); if (LOG.isDebugEnabled()) { LOG.debug("All active ledgers from ZK for hash node " + level1 + "/" + level2 + " : " + zkActiveLedgers); } - return new LedgerRange(zkActiveLedgers, - getStartLedgerIdByLevel(level1, level2), getEndLedgerIdByLevel(level1, level2)); + return new LedgerRange(zkActiveLedgers.subSet(getStartLedgerIdByLevel(level1, level2), + getEndLedgerIdByLevel(level1, level2))); } } } Modified: zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/LedgerManager.java URL: http://svn.apache.org/viewvc/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/LedgerManager.java?rev=1464385&r1=1464384&r2=1464385&view=diff ============================================================================== --- zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/LedgerManager.java (original) +++ zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/LedgerManager.java Thu Apr 4 09:21:26 2013 @@ -21,6 +21,8 @@ package org.apache.bookkeeper.meta; import java.io.Closeable; import java.io.IOException; import java.util.Set; +import java.util.SortedSet; +import java.util.TreeSet; import org.apache.zookeeper.AsyncCallback; import org.apache.bookkeeper.client.BKException; @@ -127,34 +129,23 @@ public interface LedgerManager extends C * current scan. */ public static class LedgerRange { - // ledger start and end ranges - private final long start; - private final long end; - public final static long NOLIMIT = -1; - // returned ledgers - private Set ledgers; + private final SortedSet ledgers; public LedgerRange(Set ledgers) { - this(ledgers, NOLIMIT, NOLIMIT); - } - - public LedgerRange(Set ledgers, long start) { - this(ledgers, start, NOLIMIT); + this.ledgers = new TreeSet(ledgers); } - public LedgerRange(Set ledgers, long start, long end) { - this.ledgers = ledgers; - this.start = start; - this.end = end; + public int size() { + return this.ledgers.size(); } public Long start() { - return this.start; + return ledgers.first(); } public Long end() { - return this.end; + return ledgers.last(); } public Set getLedgers() { Modified: zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/MSLedgerManagerFactory.java URL: http://svn.apache.org/viewvc/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/MSLedgerManagerFactory.java?rev=1464385&r1=1464384&r2=1464385&view=diff ============================================================================== --- zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/MSLedgerManagerFactory.java (original) +++ zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/MSLedgerManagerFactory.java Thu Apr 4 09:21:26 2013 @@ -463,6 +463,7 @@ public class MSLedgerManagerFactory exte class MSLedgerRangeIterator implements LedgerRangeIterator { final CountDownLatch openCursorLatch = new CountDownLatch(1); MetastoreCursor cursor = null; + // last ledger id in previous range MSLedgerRangeIterator() { MetastoreCallback openCursorCb = new MetastoreCallback() { @@ -480,16 +481,16 @@ public class MSLedgerManagerFactory exte } @Override - public boolean hasNext() { + public boolean hasNext() throws IOException { try { openCursorLatch.await(); } catch (InterruptedException ie) { LOG.error("Interrupted waiting for cursor to open", ie); Thread.currentThread().interrupt(); - return false; + throw new IOException("Interrupted waiting to read range", ie); } if (cursor == null) { - return false; + throw new IOException("Failed to open ledger range cursor, check logs"); } return cursor.hasMoreEntries(); } @@ -497,7 +498,7 @@ public class MSLedgerManagerFactory exte @Override public LedgerRange next() throws IOException { try { - Set ledgerIds = new TreeSet(); + SortedSet ledgerIds = new TreeSet(); Iterator iter = cursor.readEntries(maxEntriesPerScan); while (iter.hasNext()) { ledgerIds.add(key2LedgerId(iter.next().getKey())); Modified: zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/metastore/InMemoryMetastoreCursor.java URL: http://svn.apache.org/viewvc/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/metastore/InMemoryMetastoreCursor.java?rev=1464385&r1=1464384&r2=1464385&view=diff ============================================================================== --- zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/metastore/InMemoryMetastoreCursor.java (original) +++ zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/metastore/InMemoryMetastoreCursor.java Thu Apr 4 09:21:26 2013 @@ -24,22 +24,25 @@ import java.util.ArrayList; import java.util.Iterator; import java.util.List; import java.util.Map; -import java.util.NavigableMap; import java.util.Set; +import java.util.SortedMap; import java.util.concurrent.ScheduledExecutorService; import org.apache.bookkeeper.metastore.MSException.Code; import org.apache.bookkeeper.versioning.Versioned; +import com.google.common.collect.ImmutableSortedMap; + class InMemoryMetastoreCursor implements MetastoreCursor { private final ScheduledExecutorService scheduler; private final Iterator>> iter; private final Set fields; - public InMemoryMetastoreCursor(NavigableMap> map, Set fields, + public InMemoryMetastoreCursor(SortedMap> map, Set fields, ScheduledExecutorService scheduler) { - this.iter = map.entrySet().iterator(); + // copy an map for iterator to avoid concurrent modification problem. + this.iter = ImmutableSortedMap.copyOfSorted(map).entrySet().iterator(); this.fields = fields; this.scheduler = scheduler; } Modified: zookeeper/bookkeeper/trunk/bookkeeper-server/src/test/java/org/apache/bookkeeper/meta/GcLedgersTest.java URL: http://svn.apache.org/viewvc/zookeeper/bookkeeper/trunk/bookkeeper-server/src/test/java/org/apache/bookkeeper/meta/GcLedgersTest.java?rev=1464385&r1=1464384&r2=1464385&view=diff ============================================================================== --- zookeeper/bookkeeper/trunk/bookkeeper-server/src/test/java/org/apache/bookkeeper/meta/GcLedgersTest.java (original) +++ zookeeper/bookkeeper/trunk/bookkeeper-server/src/test/java/org/apache/bookkeeper/meta/GcLedgersTest.java Thu Apr 4 09:21:26 2013 @@ -27,7 +27,12 @@ import java.util.List; import java.util.HashSet; import java.util.Random; import java.util.Set; +import java.util.SortedSet; +import java.util.TreeSet; +import java.util.Queue; +import java.util.LinkedList; import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicInteger; import org.apache.bookkeeper.bookie.GarbageCollector; @@ -85,6 +90,21 @@ public class GcLedgersTest extends Ledge } } + private void removeLedger(long ledgerId) throws Exception { + final AtomicInteger rc = new AtomicInteger(0); + final CountDownLatch latch = new CountDownLatch(1); + getLedgerManager().removeLedgerMetadata(ledgerId, Version.ANY, + new GenericCallback() { + @Override + public void operationComplete(int rc2, Void result) { + rc.set(rc2); + latch.countDown(); + } + }); + assertTrue(latch.await(10, TimeUnit.SECONDS)); + assertEquals("Remove should have succeeded", 0, rc.get()); + } + @Test(timeout=60000) public void testGarbageCollectLedgers() throws Exception { int numLedgers = 100; @@ -112,7 +132,7 @@ public class GcLedgersTest extends Ledge removedLedgers.notify(); } } - }); + }); removedLedgers.wait(); } removedLedgers.add(ledgerId); @@ -175,4 +195,38 @@ public class GcLedgersTest extends Ledge assertTrue(activeLedgers.containsKey(ledger)); } } + + @Test(timeout=60000) + public void testGcLedgersOutsideRange() throws Exception { + final SortedSet createdLedgers = Collections.synchronizedSortedSet(new TreeSet()); + final Queue cleaned = new LinkedList(); + int numLedgers = 100; + + createLedgers(numLedgers, createdLedgers); + + final GarbageCollector garbageCollector = + new ScanAndCompareGarbageCollector(getLedgerManager(), activeLedgers); + GarbageCollector.GarbageCleaner cleaner = new GarbageCollector.GarbageCleaner() { + @Override + public void clean(long ledgerId) { + LOG.info("Cleaned {}", ledgerId); + cleaned.add(ledgerId); + } + }; + + garbageCollector.gc(cleaner); + assertNull("Should have cleaned nothing", cleaned.poll()); + + long last = createdLedgers.last(); + removeLedger(last); + garbageCollector.gc(cleaner); + assertNotNull("Should have cleaned something", cleaned.peek()); + assertEquals("Should have cleaned last ledger" + last, (long)last, (long)cleaned.poll()); + + long first = createdLedgers.first(); + removeLedger(first); + garbageCollector.gc(cleaner); + assertNotNull("Should have cleaned something", cleaned.peek()); + assertEquals("Should have cleaned first ledger" + first, (long)first, (long)cleaned.poll()); + } } Modified: zookeeper/bookkeeper/trunk/bookkeeper-server/src/test/java/org/apache/bookkeeper/meta/LedgerManagerTestCase.java URL: http://svn.apache.org/viewvc/zookeeper/bookkeeper/trunk/bookkeeper-server/src/test/java/org/apache/bookkeeper/meta/LedgerManagerTestCase.java?rev=1464385&r1=1464384&r2=1464385&view=diff ============================================================================== --- zookeeper/bookkeeper/trunk/bookkeeper-server/src/test/java/org/apache/bookkeeper/meta/LedgerManagerTestCase.java (original) +++ zookeeper/bookkeeper/trunk/bookkeeper-server/src/test/java/org/apache/bookkeeper/meta/LedgerManagerTestCase.java Thu Apr 4 09:21:26 2013 @@ -64,7 +64,8 @@ public abstract class LedgerManagerTestC public static Collection configs() { return Arrays.asList(new Object[][] { { FlatLedgerManagerFactory.class }, - { HierarchicalLedgerManagerFactory.class } + { HierarchicalLedgerManagerFactory.class }, + { MSLedgerManagerFactory.class } }); }