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 5cfd618f7f4..6c9c31c7449 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 @@ -5337,8 +5337,6 @@ 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 782dc9392a5..76a9d0fb544 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,11 +162,4 @@ 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 91886a8ea7b..957f4174392 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,7 +71,4 @@ 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 e0005010264..e319f909d6c 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,13 +124,24 @@ class ReversedStoreScanner extends StoreScanner implements KeyValueScanner { @Override public boolean seekToPreviousRow(Cell key) throws IOException { - checkReseek(); - return this.heap.seekToPreviousRow(key); + lock.lock(); + try { + checkReseek(); + return this.heap.seekToPreviousRow(key); + } finally { + lock.unlock(); + } + } @Override public boolean backwardSeek(Cell key) throws IOException { - checkReseek(); - return this.heap.backwardSeek(key); + lock.lock(); + try { + checkReseek(); + return this.heap.backwardSeek(key); + } finally { + lock.unlock(); + } } } 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 bac6f5c9ee7..a8ee091d344 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,7 +489,4 @@ 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 ff89b52b377..7ce4e0b91cd 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,6 +25,7 @@ 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,17 +103,10 @@ 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, @@ -401,10 +395,15 @@ 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 @@ -415,6 +414,8 @@ 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,13 +425,21 @@ 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(); + } } /** @@ -455,6 +464,8 @@ 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); } @@ -606,6 +617,9 @@ public class StoreScanner extends NonReversedNonLazyKeyValueScanner // No more keys close(); return NextState.makeState(NextState.State.NO_MORE_VALUES, totalHeapSize); + } finally { + lock.unlock(); + } } /* @@ -649,7 +663,8 @@ public class StoreScanner extends NonReversedNonLazyKeyValueScanner // Implementation of ChangedReadersObserver @Override public void updateReaders() throws IOException { - synchronized(readerLock) { + lock.lock(); + try { if (this.closing) return; // All public synchronized API calls will call 'checkReseek' which will cause @@ -669,6 +684,8 @@ 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(); } } @@ -759,6 +776,8 @@ 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. @@ -767,6 +786,9 @@ public class StoreScanner extends NonReversedNonLazyKeyValueScanner return heap.requestSeek(kv, true, useRowColBloom); } return heap.reseek(kv); + } finally { + lock.unlock(); + } } @Override @@ -841,9 +863,5 @@ public class StoreScanner extends NonReversedNonLazyKeyValueScanner public Cell getNextIndexedKey() { return this.heap.getNextIndexedKey(); } - - @Override - public void setReaderLock(Object obj) { - this.readerLock = obj; - } } +