hbase-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jean-Daniel Cryans" <jdcry...@apache.org>
Subject Re: Review Request: HBASE-2915 Deadlock between HRegion.ICV and HRegion.close
Date Mon, 23 Aug 2010 16:52:44 GMT


> On 2010-08-20 20:49:15, stack wrote:
> > /trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java, line 716
> > <http://review.cloudera.org/r/691/diff/2/?file=7680#file7680line716>
> >
> >     Any reason this does not follow the pattern used elsewhere?  You are not testing
closing before getting the read lock?

mmm didn't want to copy the same chunk of code over there, but yeah it will also give us a
fail-fast behavior which will speed up flushing.


> On 2010-08-20 20:49:15, stack wrote:
> > /trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java, line 785
> > <http://review.cloudera.org/r/691/diff/2/?file=7680#file7680line785>
> >
> >     ditto

ditto


> On 2010-08-20 20:49:15, stack wrote:
> > /trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java, line 2960
> > <http://review.cloudera.org/r/691/diff/2/?file=7680#file7680line2960>
> >
> >     Is this supposed to be inside the try?

Per that method's javadoc, no. The reason is that if it's all in a try block, and that closing
is true, then you won't hold a readLock and you can't do a isHeldByCurrent thread on that
lock. So I thought we could catch IllegalStateException in closeRegionOperation, but that
would be really ugly. This leaves us to the current situation where if an exception is thrown
when we are on this line:
     if (this.closed.get()) {
that it would leave the readLock locked by the thread, although in this case the region is
closing so we're getting rid of it anyways. Although, thinking about it, I should probably
do this instead:

    if (this.closed.get()) {
      lock.readLock().unlock();
      throw new NotServingRegionException(regionInfo.getRegionNameAsString() +
          " is closed");
    }


- Jean-Daniel


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.cloudera.org/r/691/#review988
-----------------------------------------------------------


On 2010-08-20 17:43:52, Jean-Daniel Cryans wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://review.cloudera.org/r/691/
> -----------------------------------------------------------
> 
> (Updated 2010-08-20 17:43:52)
> 
> 
> Review request for hbase.
> 
> 
> Summary
> -------
> 
> This patch removes newScannerLock and renames splitAndClose lock to just "lock". Every
operation is now required to obtain the read lock on "lock" before doing anything (including
getting a row lock). This is done by calling openRegionTransaction inside a try statement
and by calling closeRegionTransaction in finally.
> 
> flushcache got refactored some more in order to do the locking in the proper order; first
get the read lock, then do the writestate handling.
> 
> Finally, it removes the need to have a writeLock when flushing when subclassers give
atomic work do to via internalPreFlushcacheCommit. This means that this patch breaks external
contribs. This is required to keep our whole locking mechanism simpler.
> 
> 
> This addresses bug HBASE-2915.
>     http://issues.apache.org/jira/browse/HBASE-2915
> 
> 
> Diffs
> -----
> 
>   /trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 987355 
>   /trunk/src/main/java/org/apache/hadoop/hbase/regionserver/SplitTransaction.java 987355

>   /trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestSplitTransaction.java
987355 
> 
> Diff: http://review.cloudera.org/r/691/diff
> 
> 
> Testing
> -------
> 
> 5 concurrent ICV threads + randomWrite 3 + scans on a single RS. I'm also in the process
of deploying it on a cluster.
> 
> 
> Thanks,
> 
> Jean-Daniel
> 
>


Mime
View raw message