hbase-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From st...@duboce.net
Subject Re: Review Request: HBASE-2915 Deadlock between HRegion.ICV and HRegion.close
Date Fri, 20 Aug 2010 23:33:57 GMT


> On 2010-08-20 15:57:34, stack wrote:
> > /trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java, line 507
> > <http://review.cloudera.org/r/691/diff/1/?file=7612#file7612line507>
> >
> >     I suppose this order is ok if the first thing we do on entrance to HRegion is
get the read lock before check of closing.
> 
> Jean-Daniel Cryans wrote:
>     So I just redid that part. setClosing is first taken so that when the client threads
arrive they can fast fail by looking at closing.get before trying to get the readLock.
> 
> stack wrote:
>     Don't you have to check again the setClosing after you get the read lock?
> 
> Jean-Daniel Cryans wrote:
>     I'm about to post a new patch, but it looks like this and it has to be called just
before "try" instead of inside:
>          if (this.closing.get()) {
>           throw new NotServingRegionException(regionInfo.getRegionNameAsString() +
>               " is closing");
>         }
>         lock.readLock().lock();
>         if (this.closed.get()) {
>           throw new NotServingRegionException(regionInfo.getRegionNameAsString() +
>               " is closed");
>         }

That looks right.


> On 2010-08-20 15:57:34, stack wrote:
> > /trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java, line 712
> > <http://review.cloudera.org/r/691/diff/1/?file=7612#file7612line712>
> >
> >     Seems like you could use your opentransaction/closetransaction methods here
and in flush too to be consistent?
> 
> Jean-Daniel Cryans wrote:
>     Yeah the issue with compact and flush is that the callers don't expect to see NSRE,
the want null values.
> 
> stack wrote:
>     OK. Not important. This is deep internal stuff or make a version that takes a flag
on whether to throw exception (default throws exception .. might get messy though... not important).
> 
> Jean-Daniel Cryans wrote:
>     I'm afraid those little methods could be clogged fast.

Yeah. Not important.


- stack


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


On 2010-08-19 14:59:41, Jean-Daniel Cryans wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://review.cloudera.org/r/691/
> -----------------------------------------------------------
> 
> (Updated 2010-08-19 14:59:41)
> 
> 
> 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 987300 
>   /trunk/src/main/java/org/apache/hadoop/hbase/regionserver/SplitTransaction.java 987300

>   /trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestSplitTransaction.java
987300 
> 
> 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