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 79cb7db..b2b0e21 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 @@ -5008,6 +5008,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 6eba203..1ed909a 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 @@ -156,4 +156,11 @@ public interface KeyValueScanner { * @throws IOException */ public boolean seekToLastRow() throws IOException; + + /** + * Set the object to lock when the scanners 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 9dc46ce..b1b6b05 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 @@ -67,4 +67,7 @@ public abstract class NonLazyKeyValueScanner implements KeyValueScanner { // Not a file by default. return false; } + + @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 8f494c0..cde8765 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 @@ -484,4 +484,7 @@ public class StoreFileScanner implements KeyValueScanner { } return true; } + + @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 9db116e..740f806 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; @@ -102,10 +101,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, @@ -394,15 +400,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 @@ -413,8 +414,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 @@ -424,21 +423,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(); - } } /** @@ -449,8 +440,6 @@ public class StoreScanner extends NonReversedNonLazyKeyValueScanner */ @Override public boolean next(List outResult, int limit) throws IOException { - lock.lock(); - try { if (checkReseek()) { return true; } @@ -591,9 +580,6 @@ public class StoreScanner extends NonReversedNonLazyKeyValueScanner // No more keys close(); return false; - } finally { - lock.unlock(); - } } @Override @@ -604,8 +590,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 @@ -625,8 +610,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(); } } @@ -717,8 +700,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. @@ -727,9 +708,6 @@ public class StoreScanner extends NonReversedNonLazyKeyValueScanner return heap.requestSeek(kv, true, useRowColBloom); } return heap.reseek(kv); - } finally { - lock.unlock(); - } } @Override @@ -799,5 +777,9 @@ public class StoreScanner extends NonReversedNonLazyKeyValueScanner public long getEstimatedNumberOfKvsScanned() { return this.kvsScanned; } -} + @Override + public void setReaderLock(Object obj) { + this.readerLock = obj; + } +}