You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@hbase.apache.org by ni...@apache.org on 2024/09/04 15:43:25 UTC
(hbase) branch branch-3 updated: HBASE-27903 Skip submitting Split/Merge procedure when split/merge is disabled at table level (#6169) (#6185)
This is an automated email from the ASF dual-hosted git repository.
nihaljain pushed a commit to branch branch-3
in repository https://gitbox.apache.org/repos/asf/hbase.git
The following commit(s) were added to refs/heads/branch-3 by this push:
new a1fc641a3e9 HBASE-27903 Skip submitting Split/Merge procedure when split/merge is disabled at table level (#6169) (#6185)
a1fc641a3e9 is described below
commit a1fc641a3e9f4d501d3df5dbb5edd81ece2bc919
Author: Nihal Jain <ni...@apache.org>
AuthorDate: Wed Sep 4 21:13:17 2024 +0530
HBASE-27903 Skip submitting Split/Merge procedure when split/merge is disabled at table level (#6169) (#6185)
- Fail fast by adding a check before even submitting a procedure
- Update test cases to assert for expected exception post this change
- Remove deprecated method mergeRegionsAsync's usage in test
- Make use of RegionInfo.getShortNameToLog instead of logging complete region info
- Update comments in procedure implementation
Signed-off-by: Duo Zhang <zh...@apache.org>
(cherry picked from commit 2ac657caa5143c4fa36b7322d0636a51b2c235dc)
---
.../org/apache/hadoop/hbase/master/HMaster.java | 23 +++++++++++----
.../assignment/MergeTableRegionsProcedure.java | 33 +++++++++++-----------
.../assignment/SplitTableRegionProcedure.java | 10 ++++---
.../hbase/client/TestSplitOrMergeAtTableLevel.java | 10 ++++---
4 files changed, 46 insertions(+), 30 deletions(-)
diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java
index c263a383baf..e3c56b4df8f 100644
--- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java
+++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java
@@ -2256,21 +2256,26 @@ public class HMaster extends HBaseServerBase<MasterRpcServices> implements Maste
final long nonce) throws IOException {
checkInitialized();
+ final String regionNamesToLog = RegionInfo.getShortNameToLog(regionsToMerge);
+
if (!isSplitOrMergeEnabled(MasterSwitchType.MERGE)) {
- String regionsStr = Arrays.deepToString(regionsToMerge);
- LOG.warn("Merge switch is off! skip merge of " + regionsStr);
+ LOG.warn("Merge switch is off! skip merge of " + regionNamesToLog);
+ throw new DoNotRetryIOException(
+ "Merge of " + regionNamesToLog + " failed because merge switch is off");
+ }
+
+ if (!getTableDescriptors().get(regionsToMerge[0].getTable()).isMergeEnabled()) {
+ LOG.warn("Merge is disabled for the table! Skipping merge of {}", regionNamesToLog);
throw new DoNotRetryIOException(
- "Merge of " + regionsStr + " failed because merge switch is off");
+ "Merge of " + regionNamesToLog + " failed as region merge is disabled for the table");
}
- final String mergeRegionsStr = Arrays.stream(regionsToMerge).map(RegionInfo::getEncodedName)
- .collect(Collectors.joining(", "));
return MasterProcedureUtil.submitProcedure(new NonceProcedureRunnable(this, ng, nonce) {
@Override
protected void run() throws IOException {
getMaster().getMasterCoprocessorHost().preMergeRegions(regionsToMerge);
String aid = getClientIdAuditPrefix();
- LOG.info("{} merge regions {}", aid, mergeRegionsStr);
+ LOG.info("{} merge regions {}", aid, regionNamesToLog);
submitProcedure(new MergeTableRegionsProcedure(procedureExecutor.getEnvironment(),
regionsToMerge, forcible));
getMaster().getMasterCoprocessorHost().postMergeRegions(regionsToMerge);
@@ -2294,6 +2299,12 @@ public class HMaster extends HBaseServerBase<MasterRpcServices> implements Maste
"Split region " + regionInfo.getRegionNameAsString() + " failed due to split switch off");
}
+ if (!getTableDescriptors().get(regionInfo.getTable()).isSplitEnabled()) {
+ LOG.warn("Split is disabled for the table! Skipping split of {}", regionInfo);
+ throw new DoNotRetryIOException("Split region " + regionInfo.getRegionNameAsString()
+ + " failed as region split is disabled for the table");
+ }
+
return MasterProcedureUtil
.submitProcedure(new MasterProcedureUtil.NonceProcedureRunnable(this, nonceGroup, nonce) {
@Override
diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/MergeTableRegionsProcedure.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/MergeTableRegionsProcedure.java
index b9a3ee13361..201fc732175 100644
--- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/MergeTableRegionsProcedure.java
+++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/MergeTableRegionsProcedure.java
@@ -443,27 +443,28 @@ public class MergeTableRegionsProcedure
private boolean prepareMergeRegion(final MasterProcedureEnv env) throws IOException {
// Fail if we are taking snapshot for the given table
TableName tn = regionsToMerge[0].getTable();
+ final String regionNamesToLog = RegionInfo.getShortNameToLog(regionsToMerge);
if (env.getMasterServices().getSnapshotManager().isTableTakingAnySnapshot(tn)) {
- throw new MergeRegionException("Skip merging regions "
- + RegionInfo.getShortNameToLog(regionsToMerge) + ", because we are snapshotting " + tn);
+ throw new MergeRegionException(
+ "Skip merging regions " + regionNamesToLog + ", because we are snapshotting " + tn);
}
- // Mostly this check is not used because we already check the switch before submit a merge
- // procedure. Just for safe, check the switch again. This procedure can be rollbacked if
- // the switch was set to false after submit.
+ // Mostly the below two checks are not used because we already check the switches before
+ // submitting the merge procedure. Just for safety, we are checking the switch again here.
+ // Also, in case the switch was set to false after submission, this procedure can be rollbacked,
+ // thanks to this double check!
+ // case 1: check for cluster level switch
if (!env.getMasterServices().isSplitOrMergeEnabled(MasterSwitchType.MERGE)) {
- String regionsStr = Arrays.deepToString(this.regionsToMerge);
- LOG.warn("Merge switch is off! skip merge of " + regionsStr);
+ LOG.warn("Merge switch is off! skip merge of " + regionNamesToLog);
setFailure(getClass().getSimpleName(),
- new IOException("Merge of " + regionsStr + " failed because merge switch is off"));
+ new IOException("Merge of " + regionNamesToLog + " failed because merge switch is off"));
return false;
}
-
+ // case 2: check for table level switch
if (!env.getMasterServices().getTableDescriptors().get(getTableName()).isMergeEnabled()) {
- String regionsStr = Arrays.deepToString(regionsToMerge);
- LOG.warn("Merge is disabled for the table! Skipping merge of {}", regionsStr);
+ LOG.warn("Merge is disabled for the table! Skipping merge of {}", regionNamesToLog);
setFailure(getClass().getSimpleName(), new IOException(
- "Merge of " + regionsStr + " failed as region merge is disabled for the table"));
+ "Merge of " + regionNamesToLog + " failed as region merge is disabled for the table"));
return false;
}
@@ -471,8 +472,8 @@ public class MergeTableRegionsProcedure
RegionStateStore regionStateStore = env.getAssignmentManager().getRegionStateStore();
for (RegionInfo ri : this.regionsToMerge) {
if (regionStateStore.hasMergeRegions(ri)) {
- String msg = "Skip merging " + RegionInfo.getShortNameToLog(regionsToMerge)
- + ", because a parent, " + RegionInfo.getShortNameToLog(ri) + ", has a merge qualifier "
+ String msg = "Skip merging " + regionNamesToLog + ", because a parent, "
+ + RegionInfo.getShortNameToLog(ri) + ", has a merge qualifier "
+ "(if a 'merge column' in parent, it was recently merged but still has outstanding "
+ "references to its parents that must be cleared before it can participate in merge -- "
+ "major compact it to hurry clearing of its references)";
@@ -492,8 +493,8 @@ public class MergeTableRegionsProcedure
try {
if (!isMergeable(env, state)) {
setFailure(getClass().getSimpleName(),
- new MergeRegionException("Skip merging " + RegionInfo.getShortNameToLog(regionsToMerge)
- + ", because a parent, " + RegionInfo.getShortNameToLog(ri) + ", is not mergeable"));
+ new MergeRegionException("Skip merging " + regionNamesToLog + ", because a parent, "
+ + RegionInfo.getShortNameToLog(ri) + ", is not mergeable"));
return false;
}
} catch (IOException e) {
diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/SplitTableRegionProcedure.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/SplitTableRegionProcedure.java
index afa0f5e42b0..01cd012ddad 100644
--- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/SplitTableRegionProcedure.java
+++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/SplitTableRegionProcedure.java
@@ -547,16 +547,18 @@ public class SplitTableRegionProcedure
return false;
}
- // Mostly this check is not used because we already check the switch before submit a split
- // procedure. Just for safe, check the switch again. This procedure can be rollbacked if
- // the switch was set to false after submit.
+ // Mostly the below two checks are not used because we already check the switches before
+ // submitting the split procedure. Just for safety, we are checking the switch again here.
+ // Also, in case the switch was set to false after submission, this procedure can be rollbacked,
+ // thanks to this double check!
+ // case 1: check for cluster level switch
if (!env.getMasterServices().isSplitOrMergeEnabled(MasterSwitchType.SPLIT)) {
LOG.warn("pid=" + getProcId() + " split switch is off! skip split of " + parentHRI);
setFailure(new IOException(
"Split region " + parentHRI.getRegionNameAsString() + " failed due to split switch off"));
return false;
}
-
+ // case 2: check for table level switch
if (!env.getMasterServices().getTableDescriptors().get(getTableName()).isSplitEnabled()) {
LOG.warn("pid={}, split is disabled for the table! Skipping split of {}", getProcId(),
parentHRI);
diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestSplitOrMergeAtTableLevel.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestSplitOrMergeAtTableLevel.java
index d535fc54356..c5c31c1d6fd 100644
--- a/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestSplitOrMergeAtTableLevel.java
+++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestSplitOrMergeAtTableLevel.java
@@ -25,6 +25,7 @@ import java.util.List;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.Future;
import java.util.concurrent.TimeUnit;
+import org.apache.hadoop.hbase.DoNotRetryIOException;
import org.apache.hadoop.hbase.HBaseClassTestRule;
import org.apache.hadoop.hbase.HBaseTestingUtil;
import org.apache.hadoop.hbase.TableName;
@@ -32,7 +33,6 @@ import org.apache.hadoop.hbase.Waiter.ExplainingPredicate;
import org.apache.hadoop.hbase.testclassification.ClientTests;
import org.apache.hadoop.hbase.testclassification.MediumTests;
import org.apache.hadoop.hbase.util.Bytes;
-import org.apache.hadoop.hbase.util.Threads;
import org.junit.AfterClass;
import org.junit.BeforeClass;
import org.junit.ClassRule;
@@ -125,7 +125,7 @@ public class TestSplitOrMergeAtTableLevel {
assertFalse(admin.getDescriptor(tableName).isMergeEnabled());
trySplitAndEnsureItIsSuccess(tableName);
- Threads.sleep(10000);
+
tryMergeAndEnsureItFails(tableName);
admin.disableTable(tableName);
enableTableMerge(tableName);
@@ -166,6 +166,7 @@ public class TestSplitOrMergeAtTableLevel {
// expected to reach here
// check and ensure that table does not get splitted
assertTrue(admin.getRegions(tableName).size() == originalCount);
+ assertTrue("Expected DoNotRetryIOException!", ee.getCause() instanceof DoNotRetryIOException);
}
}
@@ -217,7 +218,7 @@ public class TestSplitOrMergeAtTableLevel {
byte[] nameOfRegionB = regions.get(1).getEncodedNameAsBytes();
// check and ensure that region do not get merged
- Future<?> f = admin.mergeRegionsAsync(nameOfRegionA, nameOfRegionB, true);
+ Future<?> f = admin.mergeRegionsAsync(new byte[][] { nameOfRegionA, nameOfRegionB }, true);
try {
f.get(10, TimeUnit.SECONDS);
fail("Should not get here.");
@@ -225,6 +226,7 @@ public class TestSplitOrMergeAtTableLevel {
// expected to reach here
// check and ensure that region do not get merged
assertTrue(admin.getRegions(tableName).size() == originalCount);
+ assertTrue("Expected DoNotRetryIOException!", ee.getCause() instanceof DoNotRetryIOException);
}
}
@@ -255,7 +257,7 @@ public class TestSplitOrMergeAtTableLevel {
byte[] nameOfRegionB = regions.get(1).getEncodedNameAsBytes();
// merge the table regions and wait until region count decreases
- admin.mergeRegionsAsync(nameOfRegionA, nameOfRegionB, true);
+ admin.mergeRegionsAsync(new byte[][] { nameOfRegionA, nameOfRegionB }, true);
TEST_UTIL.waitFor(30000, new ExplainingPredicate<Exception>() {
@Override