diff --git a/CHANGES.txt b/CHANGES.txt index 86adb83144f..300564bd851 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -506,6 +506,7 @@ Release 0.21.0 - Unreleased HBASE-2925 LRU of HConnectionManager.HBASE_INSTANCES breaks if HBaseConfiguration is changed (Robert Mahfoud via Stack) + HBASE-2964 Deadlock when RS tries to RPC to itself inside SplitTransaction IMPROVEMENTS HBASE-1760 Cleanup TODOs in HTable diff --git a/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java b/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java index 6b8eac8c76d..4c8bd4c9cfc 100644 --- a/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java +++ b/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java @@ -214,7 +214,7 @@ public class HRegion implements HeapSize { // , Writable{ final FlushRequester flushRequester; private final long blockingMemStoreSize; final long threadWakeFrequency; - // Used to guard splits and closes + // Used to guard closes final ReentrantReadWriteLock lock = new ReentrantReadWriteLock(); diff --git a/src/main/java/org/apache/hadoop/hbase/regionserver/SplitTransaction.java b/src/main/java/org/apache/hadoop/hbase/regionserver/SplitTransaction.java index 87c03bcb9cb..bb3b382a5da 100644 --- a/src/main/java/org/apache/hadoop/hbase/regionserver/SplitTransaction.java +++ b/src/main/java/org/apache/hadoop/hbase/regionserver/SplitTransaction.java @@ -61,6 +61,8 @@ import org.apache.zookeeper.KeeperException; * } * } * + *

This class is not thread safe. Caller needs ensure split is run by + * one thread only. */ class SplitTransaction { private static final Log LOG = LogFactory.getLog(SplitTransaction.class); @@ -127,36 +129,26 @@ class SplitTransaction { /** * Does checks on split inputs. * @return true if the region is splittable else - * false if it is not (e.g. its already closed, etc.). If we - * return true, we'll have taken out the parent's - * splitsAndClosesLock and only way to unlock is successful - * {@link #execute(OnlineRegions)} or {@link #rollback(OnlineRegions)} + * false if it is not (e.g. its already closed, etc.). */ public boolean prepare() { - boolean prepared = false; - this.parent.lock.writeLock().lock(); - try { - if (this.parent.isClosed() || this.parent.isClosing()) return prepared; - HRegionInfo hri = this.parent.getRegionInfo(); - // Check splitrow. - byte [] startKey = hri.getStartKey(); - byte [] endKey = hri.getEndKey(); - if (Bytes.equals(startKey, splitrow) || - !this.parent.getRegionInfo().containsRow(splitrow)) { - LOG.info("Split row is not inside region key range or is equal to " + + if (this.parent.isClosed() || this.parent.isClosing()) return false; + HRegionInfo hri = this.parent.getRegionInfo(); + // Check splitrow. + byte [] startKey = hri.getStartKey(); + byte [] endKey = hri.getEndKey(); + if (Bytes.equals(startKey, splitrow) || + !this.parent.getRegionInfo().containsRow(splitrow)) { + LOG.info("Split row is not inside region key range or is equal to " + "startkey: " + Bytes.toString(this.splitrow)); - return prepared; - } - long rid = getDaughterRegionIdTimestamp(hri); - this.hri_a = new HRegionInfo(hri.getTableDesc(), startKey, this.splitrow, - false, rid); - this.hri_b = new HRegionInfo(hri.getTableDesc(), this.splitrow, endKey, - false, rid); - prepared = true; - } finally { - if (!prepared) this.parent.lock.writeLock().unlock(); + return false; } - return prepared; + long rid = getDaughterRegionIdTimestamp(hri); + this.hri_a = new HRegionInfo(hri.getTableDesc(), startKey, this.splitrow, + false, rid); + this.hri_b = new HRegionInfo(hri.getTableDesc(), this.splitrow, endKey, + false, rid); + return true; } /** @@ -188,9 +180,7 @@ class SplitTransaction { final RegionServerServices services) throws IOException { LOG.info("Starting split of region " + this.parent); - if (!this.parent.lock.writeLock().isHeldByCurrentThread()) { - throw new SplitAndCloseWriteLockNotHeld(); - } + assert !this.parent.lock.writeLock().isHeldByCurrentThread() : "Unsafe to hold write lock while performing RPCs"; // If true, no cluster to write meta edits into. boolean testing = @@ -251,9 +241,6 @@ class SplitTransaction { } } - // Unlock if successful split. - this.parent.lock.writeLock().unlock(); - // Leaving here, the splitdir with its dross will be in place but since the // split was successful, just leave it; it'll be cleaned when parent is // deleted and cleaned up. @@ -441,9 +428,6 @@ class SplitTransaction { * @throws IOException If thrown, rollback failed. Take drastic action. */ public void rollback(final OnlineRegions or) throws IOException { - if (!this.parent.lock.writeLock().isHeldByCurrentThread()) { - throw new SplitAndCloseWriteLockNotHeld(); - } FileSystem fs = this.parent.getFilesystem(); ListIterator iterator = this.journal.listIterator(this.journal.size()); @@ -481,17 +465,8 @@ class SplitTransaction { throw new RuntimeException("Unhandled journal entry: " + je); } } - if (this.parent.lock.writeLock().isHeldByCurrentThread()) { - this.parent.lock.writeLock().unlock(); - } } - /** - * Thrown if lock not held. - */ - @SuppressWarnings("serial") - public class SplitAndCloseWriteLockNotHeld extends IOException {} - HRegionInfo getFirstDaughter() { return hri_a; } diff --git a/src/test/java/org/apache/hadoop/hbase/regionserver/TestSplitTransaction.java b/src/test/java/org/apache/hadoop/hbase/regionserver/TestSplitTransaction.java index 3bb0b58ecb1..67a7089ae3e 100644 --- a/src/test/java/org/apache/hadoop/hbase/regionserver/TestSplitTransaction.java +++ b/src/test/java/org/apache/hadoop/hbase/regionserver/TestSplitTransaction.java @@ -95,8 +95,6 @@ public class TestSplitTransaction { private SplitTransaction prepareGOOD_SPLIT_ROW() { SplitTransaction st = new SplitTransaction(this.parent, GOOD_SPLIT_ROW); assertTrue(st.prepare()); - // Assert the write lock is held on successful prepare as the javadoc asserts. - assertTrue(this.parent.lock.writeLock().isHeldByCurrentThread()); return st; }