Details

Description

In the one region multi assigned case, we could find that two regions have the same table name, same startKey, same endKey, and different regionId, so these two regions are same in TreeMap but different in HashMap.

-1 tests included. The patch doesn't appear to include any new or modified tests.
Please justify why no new tests are needed for this patch.
Also please list what manual steps were performed to verify this patch.

stack
added a comment - 12/Mar/12 18:31 Both regions were online?
Does this patch make the newest made region – the one with the larger regionid – sort AFTER the region with the lesser regionid? I think it does but am not sure.
+1 on this patch...

@stack
Because of some bug, we find same region assigned twice on two RS, however these two regions will both be splitted and the new four regions has the same startKey and endKey, caused serious unconsistent.

I find the one with the larger regionid should sort before the region with the lesser regionid, otherwise AssignmentManager#getRegionsOfTable() will return error regions.

chunhui shen
added a comment - 13/Mar/12 03:41 @stack
Because of some bug, we find same region assigned twice on two RS, however these two regions will both be splitted and the new four regions has the same startKey and endKey, caused serious unconsistent.
I find the one with the larger regionid should sort before the region with the lesser regionid, otherwise AssignmentManager#getRegionsOfTable() will return error regions.
Could run hadoopqa with patchv2 again?
Thanks.

Check out the review patch in HBASE-5128 (its big but in there) – there is a fix for TestRegionObserverInterface in that patch which can probably go over here. We need to look into the other faliures as well (the one I fixed was a Medium test – the others are likely Large tests that aren't run until small and medium pass)

Jonathan Hsieh
added a comment - 14/Mar/12 00:37 @chunhui
Check out the review patch in HBASE-5128 (its big but in there) – there is a fix for TestRegionObserverInterface in that patch which can probably go over here. We need to look into the other faliures as well (the one I fixed was a Medium test – the others are likely Large tests that aren't run until small and medium pass)

Jonathan Hsieh
added a comment - 14/Mar/12 21:31 @Chenhui – lets keep this open amd take care of this issue particular here. The HBASE-5128 is too big already and this a more manageable hunk to review / commit.
I'm starting to hunt down the failures new – I think TestClassLoading is intermittent.

Hey Chenhui, looked at your code and had a question (similar to stack's) – did you intend to have newer regions (larger timestamps) sorted before older HRegionInfos? If so why? My intuition says to to have newer regions sorted after older ones. At the moment nothing depends on one order vs another, so I would like to add some simple unit tests to make the desired behavior this explicitly tested.

Also, I have a few runs with the broken test passing now (a bunch of tests seem to be flakey on my setup).

Jonathan Hsieh
added a comment - 15/Mar/12 15:18 Hey Chenhui, looked at your code and had a question (similar to stack's) – did you intend to have newer regions (larger timestamps) sorted before older HRegionInfos? If so why? My intuition says to to have newer regions sorted after older ones. At the moment nothing depends on one order vs another, so I would like to add some simple unit tests to make the desired behavior this explicitly tested.
Also, I have a few runs with the broken test passing now (a bunch of tests seem to be flakey on my setup).

Hadoop QA
added a comment - 16/Mar/12 00:36 -1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12518570/hbase-5563-v3-0.92.patch
against trunk revision .
+1 @author. The patch does not contain any @author tags.
+1 tests included. The patch appears to include 3 new or modified tests.
-1 patch. The patch command could not apply the patch.
Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1196//console
This message is automatically generated.

chunhui shen
added a comment - 16/Mar/12 01:45 @Jonathan
have newer regions (larger timestamps) sorted before older HRegionInfos? If so why? My intuition says to to have newer regions sorted after older ones.
I just in order to make AssignmentManager#getRegionsOfTable() returns correct without changing it.

I get it – we just didn't see the robot run the v2 to show that it fixed the problem.

Are you ok with the newer patch (and test?) that fixes getRegionsOfTable? I think it is more intuitive since older HRI's with smaller datestamp/regionIds are smaller than newer HRI's with larger datestamp/regionIds.

It's helpful to add comments and tests to show what you intend – hopefully the updated patch I provided makes it clear.

Jonathan Hsieh
added a comment - 16/Mar/12 13:56 @Chunhui
I get it – we just didn't see the robot run the v2 to show that it fixed the problem.
Are you ok with the newer patch (and test?) that fixes getRegionsOfTable? I think it is more intuitive since older HRI's with smaller datestamp/regionIds are smaller than newer HRI's with larger datestamp/regionIds.
It's helpful to add comments and tests to show what you intend – hopefully the updated patch I provided makes it clear.