commit ec1eff9b69d37d2340f057361d2b269cc5fffd56 Author: Lars Hofhansl Date: Fri Mar 6 12:44:24 2015 -0800 HBASE-13082 Coarsen StoreScanner locks to RegionScanner. diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java index 13569d7..830907d 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java @@ -5233,6 +5233,8 @@ public class HRegion implements HeapSize, PropagatingConfigurationObserver { // scan.getFamilyMap().entrySet()) { Store store = stores.get(entry.getKey()); KeyValueScanner scanner = store.getScanner(scan, entry.getValue(), this.readPt); + // pass the RegionScanner object to lock out concurrent changes to set of readers + scanner.setReaderLock(this); if (this.filter == null || !scan.doLoadColumnFamiliesOnDemand() || this.filter.isFamilyEssential(entry.getKey())) { scanners.add(scanner); diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/KeyValueScanner.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/KeyValueScanner.java index 76a9d0f..782dc93 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/KeyValueScanner.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/KeyValueScanner.java @@ -162,4 +162,11 @@ public interface KeyValueScanner { * if known, or null otherwise */ public Cell getNextIndexedKey(); + + /** + * Set the object to lock when the scanner's readers (if any) are updated (this is here so that the + * coprocessors creating {@link StoreScanner}'s do not have to change) + * @param obj lock object to use + */ + public void setReaderLock(Object obj); } diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/NonLazyKeyValueScanner.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/NonLazyKeyValueScanner.java index 957f417..91886a8 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/NonLazyKeyValueScanner.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/NonLazyKeyValueScanner.java @@ -71,4 +71,7 @@ public abstract class NonLazyKeyValueScanner implements KeyValueScanner { public Cell getNextIndexedKey() { return null; } + + @Override + public void setReaderLock(Object obj) {/* NO OP */} } diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/ReversedStoreScanner.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/ReversedStoreScanner.java index e319f90..e000501 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/ReversedStoreScanner.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/ReversedStoreScanner.java @@ -124,24 +124,13 @@ class ReversedStoreScanner extends StoreScanner implements KeyValueScanner { @Override public boolean seekToPreviousRow(Cell key) throws IOException { - lock.lock(); - try { - checkReseek(); - return this.heap.seekToPreviousRow(key); - } finally { - lock.unlock(); - } - + checkReseek(); + return this.heap.seekToPreviousRow(key); } @Override public boolean backwardSeek(Cell key) throws IOException { - lock.lock(); - try { - checkReseek(); - return this.heap.backwardSeek(key); - } finally { - lock.unlock(); - } + checkReseek(); + return this.heap.backwardSeek(key); } } diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFileScanner.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFileScanner.java index a8ee091..bac6f5c 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFileScanner.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFileScanner.java @@ -489,4 +489,7 @@ public class StoreFileScanner implements KeyValueScanner { public Cell getNextIndexedKey() { return hfs.getNextIndexedKey(); } + + @Override + public void setReaderLock(Object obj) {/* NO OP */} } diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java index 7ce4e0b..ff89b52 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java @@ -25,7 +25,6 @@ import java.util.ArrayList; import java.util.List; import java.util.NavigableSet; import java.util.concurrent.CountDownLatch; -import java.util.concurrent.locks.ReentrantLock; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; @@ -103,10 +102,17 @@ public class StoreScanner extends NonReversedNonLazyKeyValueScanner // A flag whether use pread for scan private boolean scanUsePread = false; - protected ReentrantLock lock = new ReentrantLock(); private final long readPt; + // lock to use for updateReaders + // creator needs to ensure that: + // 1. *all* calls to public methods (except updateReaders and close) are locked with this lock + // (this can be done by passing the RegionScannerImpl object down) + // OR + // 2. updateReader is *never* called (such as in flushes or compactions) + private Object readerLock; + // used by the injection framework to test race between StoreScanner construction and compaction enum StoreScannerCompactionRace { BEFORE_SEEK, @@ -395,15 +401,10 @@ public class StoreScanner extends NonReversedNonLazyKeyValueScanner @Override public Cell peek() { - lock.lock(); - try { if (this.heap == null) { return this.lastTop; } return this.heap.peek(); - } finally { - lock.unlock(); - } } @Override @@ -414,8 +415,6 @@ public class StoreScanner extends NonReversedNonLazyKeyValueScanner @Override public void close() { - lock.lock(); - try { if (this.closing) return; this.closing = true; // under test, we dont have a this.store @@ -425,21 +424,13 @@ public class StoreScanner extends NonReversedNonLazyKeyValueScanner this.heap.close(); this.heap = null; // CLOSED! this.lastTop = null; // If both are null, we are closed. - } finally { - lock.unlock(); - } } @Override public boolean seek(Cell key) throws IOException { - lock.lock(); - try { // reset matcher state, in case that underlying store changed checkReseek(); return this.heap.seek(key); - } finally { - lock.unlock(); - } } /** @@ -464,8 +455,6 @@ public class StoreScanner extends NonReversedNonLazyKeyValueScanner @Override public NextState next(List outResult, int limit, long remainingResultSize) throws IOException { - lock.lock(); - try { if (checkReseek()) { return NextState.makeState(NextState.State.MORE_VALUES, 0); } @@ -617,9 +606,6 @@ public class StoreScanner extends NonReversedNonLazyKeyValueScanner // No more keys close(); return NextState.makeState(NextState.State.NO_MORE_VALUES, totalHeapSize); - } finally { - lock.unlock(); - } } /* @@ -663,8 +649,7 @@ public class StoreScanner extends NonReversedNonLazyKeyValueScanner // Implementation of ChangedReadersObserver @Override public void updateReaders() throws IOException { - lock.lock(); - try { + synchronized(readerLock) { if (this.closing) return; // All public synchronized API calls will call 'checkReseek' which will cause @@ -684,8 +669,6 @@ public class StoreScanner extends NonReversedNonLazyKeyValueScanner this.heap = null; // the re-seeks could be slow (access HDFS) free up memory ASAP // Let the next() call handle re-creating and seeking - } finally { - lock.unlock(); } } @@ -776,8 +759,6 @@ public class StoreScanner extends NonReversedNonLazyKeyValueScanner @Override public boolean reseek(Cell kv) throws IOException { - lock.lock(); - try { //Heap will not be null, if this is called from next() which. //If called from RegionScanner.reseek(...) make sure the scanner //stack is reset if needed. @@ -786,9 +767,6 @@ public class StoreScanner extends NonReversedNonLazyKeyValueScanner return heap.requestSeek(kv, true, useRowColBloom); } return heap.reseek(kv); - } finally { - lock.unlock(); - } } @Override @@ -863,5 +841,9 @@ public class StoreScanner extends NonReversedNonLazyKeyValueScanner public Cell getNextIndexedKey() { return this.heap.getNextIndexedKey(); } -} + @Override + public void setReaderLock(Object obj) { + this.readerLock = obj; + } +}