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