apurtell commented on a change in pull request #3402:
URL: https://github.com/apache/hbase/pull/3402#discussion_r658859197
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
##########
@@ -4566,7 +4586,7 @@ private void regionOffline(final HRegionInfo regionInfo,
final State state) {
// Tell our listeners that a region was closed
sendRegionClosedNotification(regionInfo);
// also note that all the replicas of the primary should be closed
- if (state != null && state.equals(State.SPLIT)) {
+ if (force || state != null && state.equals(State.SPLIT)) {
Review comment:
Recommended to add a set of parenthesis here, makes intent clearer:
`(force || (state != null && state.equals(State.SPLIT)))`
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
##########
@@ -4575,7 +4595,7 @@ private void regionOffline(final HRegionInfo regionInfo,
final State state) {
replicasToClose.addAll(list);
}
}
- else if (state != null && state.equals(State.MERGED)) {
+ else if (force || state != null && state.equals(State.MERGED)) {
Review comment:
Same as above
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/master/RegionStates.java
##########
@@ -713,7 +713,7 @@ public void regionOffline(
regionsInTransition.remove(encodedName);
ServerName oldServerName = regionAssignments.remove(hri);
if (oldServerName != null && serverHoldings.containsKey(oldServerName)) {
- if (newState == State.MERGED || newState == State.SPLIT
+ if (force || newState == State.MERGED || newState == State.SPLIT
Review comment:
Same
##########
File path:
hbase-server/src/test/java/org/apache/hadoop/hbase/util/TestHBaseFsck.java
##########
@@ -882,6 +882,80 @@ public void testDupeStartKey() throws Exception {
assertNoErrors(hbck2);
assertEquals(0, hbck2.getOverlapGroups(table).size());
assertEquals(ROWKEYS.length, countRows());
+
+ MiniHBaseCluster cluster = TEST_UTIL.getHBaseCluster();
+ long totalRegions = cluster.countServedRegions();
+
+ // stop a region servers and run fsck again
+ cluster.stopRegionServer(server);
+ cluster.waitForRegionServerToStop(server, 60);
+
+ // wait for all regions to come online.
+ while (cluster.countServedRegions() < totalRegions) {
+ try {
+ Thread.sleep(100);
+ } catch (InterruptedException e) {}
Review comment:
This pattern is often considered a code smell. The InterruptedException
is not handled and so the thread's interrupt status is swallowed. You would
want to at least do:
try {
Thread.sleep(100);
} catch (InterruptedException e) {
Thread.currentThread().interrupt(); // restore interrupt status
}
But given this is test code this is trying to hard. I would just let the
test method throw the exception FWIW
##########
File path:
hbase-server/src/test/java/org/apache/hadoop/hbase/util/TestHBaseFsck.java
##########
@@ -882,6 +882,80 @@ public void testDupeStartKey() throws Exception {
assertNoErrors(hbck2);
assertEquals(0, hbck2.getOverlapGroups(table).size());
assertEquals(ROWKEYS.length, countRows());
+
+ MiniHBaseCluster cluster = TEST_UTIL.getHBaseCluster();
+ long totalRegions = cluster.countServedRegions();
+
+ // stop a region servers and run fsck again
+ cluster.stopRegionServer(server);
+ cluster.waitForRegionServerToStop(server, 60);
+
+ // wait for all regions to come online.
+ while (cluster.countServedRegions() < totalRegions) {
+ try {
+ Thread.sleep(100);
+ } catch (InterruptedException e) {}
+ }
+
+ // check again after stopping a region server.
+ HBaseFsck hbck3 = doFsck(conf,false);
+ assertNoErrors(hbck3);
+ } finally {
+ cleanupTable(table);
+ }
+ }
+
+ /**
+ * This create and fixes a bad table with regions that have overlap regions.
+ */
+ @Test(timeout=180000)
+ public void testOverlapRegions() throws Exception {
+ MiniHBaseCluster cluster = TEST_UTIL.getHBaseCluster();
+ TableName table =
+ TableName.valueOf("tableOverlapRegions");
+ HRegionInfo hri;
+ ServerName server;
+ try {
+ setupTable(table);
+ assertNoErrors(doFsck(conf, false));
+ assertEquals(ROWKEYS.length, countRows());
+
+ // Now let's mess it up, by adding a region which overlaps with others
+ hri = createRegion(tbl.getTableDescriptor(), Bytes.toBytes("A2"),
Bytes.toBytes("B2"));
+ TEST_UTIL.assignRegion(hri);
+ server = regionStates.getRegionServerOfRegion(hri);
+ TEST_UTIL.assertRegionOnServer(hri, server, REGION_ONLINE_TIMEOUT);
+
+ HBaseFsck hbck = doFsck(conf, false);
+ assertErrors(hbck, new ERROR_CODE[] { ERROR_CODE.OVERLAP_IN_REGION_CHAIN,
+ ERROR_CODE.OVERLAP_IN_REGION_CHAIN });
+ assertEquals(3, hbck.getOverlapGroups(table).size());
+ assertEquals(ROWKEYS.length, countRows());
+
+ // fix the overlap regions.
+ doFsck(conf, true);
+
+ // check that the overlap regions are gone and no data loss
+ HBaseFsck hbck2 = doFsck(conf,false);
+ assertNoErrors(hbck2);
+ assertEquals(0, hbck2.getOverlapGroups(table).size());
+ assertEquals(ROWKEYS.length, countRows());
+
+ long totalRegions = cluster.countServedRegions();
+
+ // stop a region servers and run fsck again
+ cluster.stopRegionServer(server);
+ cluster.waitForRegionServerToStop(server, 60);
+
+ // wait for all regions to come online.
+ while (cluster.countServedRegions() < totalRegions) {
+ try {
+ Thread.sleep(100);
+ } catch (InterruptedException e) {}
Review comment:
Same as above, avoid this pattern. Although, I agree, this is test code,
we don't have to try _too_ hard.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@hbase.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
