Review Board 1.7.22


OpenedRegionHandler is not enforcing atomicity of the operation it is performing

Review Request #2251 - Created Oct. 6, 2011 and updated

ramkrishna vasudevan
0.92, trunk
HBASE-4540
Reviewers
hbase
jgray, stack, tedyu
hbase
Fix for handling HBASE-4539 and HBASE-4540.
Ran all the testcases.  Added one new testcase to verify OpenedRegionHandler scenarios.
Also addresses Ted's comments.
Yes
Review request changed
Updated (Oct. 8, 2011, 5:13 a.m.)
This updated patch is same as uploaded at @ 07/Oct/11 14:27
Reverted the change of passing -2 for not comparing the version and address Ted's comment to add spaces.
Ship it!
Posted (Oct. 8, 2011, 5:15 a.m.)

   

  
Ship it!
Posted (Oct. 8, 2011, 9:55 p.m.)
I'm good on commit.

Have some suggestions for future handler tests below.  I'm ok if we commit w/o addressing them here.

Nice fix Ram
We don't have this method already in our ZK* classes?
  1. @Stack
    ZKAssign() did have getDataAndWatch() that accepts stat object.  Only ZKUtil had but it returned data in bytes which had to be again converted to RegionTransitionData. 
    Hence added an utility api in ZKAssign itself and thought it may be useful in future also.
  2. Sounds good.
Do you have to spin up the cluster twice?   Could you do it once only in @BeforeClass and then shut it down in @AfterClass?  So its run once only?
Good test.

Would it be possible to test the handler without spinning up the cluster?  See TestOpenRegionHandler over under regionserver.handler in tests -- they don't spin up a cluster, just zk.  Test can run faster if no dfs+hbase.  Not important.  For the future.
  1. @Stack
    I can do like that atleast for one of the testcases in TestOpenedRegionHandler.  But i have to use the MockServer and MockRegionServices.
    I will raise one minor improvement task to do that. Currently MockServer and MockRegionServices are under regionserver.handler package but the new testcase is in master package.  So better we can move it to a test.utility package and then use it across. So i will currently go with this commit and then track the new improvement JIRA to closure.
  2. Sounds good Ram.  Yes, we should move these out if more generally useful.