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 Fri, 20 Aug 2010 23:21:37 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.

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.


> 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?

Yeah the issue with compact and flush is that the callers don't expect to see NSRE, the want
null values.


> On 2010-08-20 15:57:34, stack wrote:
> > /trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java, line 1115
> > <http://review.cloudera.org/r/691/diff/1/?file=7612#file7612line1115>
> >
> >     Aren't these lines unnecessary?  openRegionTransaction does it?

Good catch.


> On 2010-08-20 15:57:34, stack wrote:
> > /trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java, line 3142
> > <http://review.cloudera.org/r/691/diff/1/?file=7612#file7612line3142>
> >
> >     So, this javadoc is good but do you think we need some more doc?  Does there
need to be more detail on new locking regime? Maybe there is no more to be said that what
is here in this paragraph.  You've done all the work unravelling our lock mess.  With time
your nice unravelling will rot unless its clear what the pattern is.    I'm just trying to
think of ways of preventing that happening.

Yeah I'll included some more javadoc, maybe with code examples?


- Jean-Daniel


-----------------------------------------------------------
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