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