----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.cloudera.org/r/691/#review974 -----------------------------------------------------------
Ship it! +1 Nice fix. A few comments below. /trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java <http://review.cloudera.org/r/691/#comment3163> 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. /trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java <http://review.cloudera.org/r/691/#comment3164> Seems like you could use your opentransaction/closetransaction methods here and in flush too to be consistent? /trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java <http://review.cloudera.org/r/691/#comment3165> Yeah, just remove. /trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java <http://review.cloudera.org/r/691/#comment3166> Aren't these lines unnecessary? openRegionTransaction does it? /trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java <http://review.cloudera.org/r/691/#comment3167> 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. - stack 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 > >
