-
Notifications
You must be signed in to change notification settings - Fork 3.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HBASE-25922 - Disabled sanity checks ignored on snapshot restore #4533
Changes from 6 commits
5678a7b
4070d09
fc1374e
ed59341
da9ba20
09858ce
685b8f6
879ab3e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -60,6 +60,18 @@ public final class TableDescriptorChecker { | |
private TableDescriptorChecker() { | ||
} | ||
|
||
private static boolean shouldSanityCheck(final Configuration conf, final TableDescriptor td) { | ||
String tableVal = td.getValue(TABLE_SANITY_CHECKS); | ||
// If table descriptor is explicitly overriding, return the same | ||
if (tableVal != null) { | ||
return Boolean.valueOf(tableVal); | ||
} | ||
if (conf.getBoolean(TABLE_SANITY_CHECKS, DEFAULT_TABLE_SANITY_CHECKS)) { | ||
return true; | ||
} | ||
return false; | ||
} | ||
|
||
/** | ||
* Checks whether the table conforms to some sane limits, and configured values (compression, etc) | ||
* work. Throws an exception if something is wrong. | ||
|
@@ -68,15 +80,8 @@ public static void sanityCheck(final Configuration c, final TableDescriptor td) | |
throws IOException { | ||
CompoundConfiguration conf = new CompoundConfiguration().add(c).addBytesMap(td.getValues()); | ||
|
||
// Setting this to true logs the warning instead of throwing exception | ||
boolean logWarn = false; | ||
if (!conf.getBoolean(TABLE_SANITY_CHECKS, DEFAULT_TABLE_SANITY_CHECKS)) { | ||
logWarn = true; | ||
} | ||
String tableVal = td.getValue(TABLE_SANITY_CHECKS); | ||
if (tableVal != null && !Boolean.valueOf(tableVal)) { | ||
logWarn = true; | ||
} | ||
// Setting logs to warning instead of throwing exception if sanityChecks are disabled | ||
boolean logWarn = !shouldSanityCheck(conf, td); | ||
|
||
// check max file size | ||
long maxFileSizeLowerLimit = 2 * 1024 * 1024L; // 2M is the default lower limit | ||
|
@@ -107,36 +112,20 @@ public static void sanityCheck(final Configuration c, final TableDescriptor td) | |
} | ||
|
||
// check that coprocessors and other specified plugin classes can be loaded | ||
try { | ||
checkClassLoading(conf, td); | ||
} catch (Exception ex) { | ||
warnOrThrowExceptionForFailure(logWarn, ex.getMessage(), null); | ||
} | ||
checkClassLoading(conf, td); | ||
|
||
if (conf.getBoolean(MASTER_CHECK_COMPRESSION, DEFAULT_MASTER_CHECK_COMPRESSION)) { | ||
// check compression can be loaded | ||
try { | ||
checkCompression(td); | ||
} catch (IOException e) { | ||
warnOrThrowExceptionForFailure(logWarn, e.getMessage(), e); | ||
} | ||
checkCompression(conf, td); | ||
} | ||
|
||
if (conf.getBoolean(MASTER_CHECK_ENCRYPTION, DEFAULT_MASTER_CHECK_ENCRYPTION)) { | ||
// check encryption can be loaded | ||
try { | ||
checkEncryption(conf, td); | ||
} catch (IOException e) { | ||
warnOrThrowExceptionForFailure(logWarn, e.getMessage(), e); | ||
} | ||
checkEncryption(conf, td); | ||
} | ||
|
||
// Verify compaction policy | ||
try { | ||
checkCompactionPolicy(conf, td); | ||
} catch (IOException e) { | ||
warnOrThrowExceptionForFailure(false, e.getMessage(), e); | ||
} | ||
checkCompactionPolicy(conf, td); | ||
// check that we have at least 1 CF | ||
if (td.getColumnFamilyCount() == 0) { | ||
String message = "Table should have at least one column family."; | ||
|
@@ -155,6 +144,12 @@ public static void sanityCheck(final Configuration c, final TableDescriptor td) | |
warnOrThrowExceptionForFailure(false, "Meta table can't be set as read only.", null); | ||
} | ||
|
||
// check replication scope | ||
checkReplicationScope(conf, td); | ||
|
||
// check bloom filter type | ||
checkBloomFilterType(conf, td); | ||
|
||
for (ColumnFamilyDescriptor hcd : td.getColumnFamilies()) { | ||
if (hcd.getTimeToLive() <= 0) { | ||
String message = "TTL for column family " + hcd.getNameAsString() + " must be positive."; | ||
|
@@ -185,11 +180,6 @@ public static void sanityCheck(final Configuration c, final TableDescriptor td) | |
warnOrThrowExceptionForFailure(logWarn, message, null); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the place where we use logWarn in this method? Then let's move the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are multiple checks based on logWarn variable before this line (for e.g. at line no 92, 106, 127, 134 above) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, OK, in unchanged code. |
||
} | ||
|
||
// check replication scope | ||
checkReplicationScope(hcd); | ||
// check bloom filter type | ||
checkBloomFilterType(hcd); | ||
|
||
// check data replication factor, it can be 0(default value) when user has not explicitly | ||
// set the value, in this case we use default replication factor set in the file system. | ||
if (hcd.getDFSReplication() < 0) { | ||
|
@@ -207,103 +197,144 @@ public static void sanityCheck(final Configuration c, final TableDescriptor td) | |
} | ||
} | ||
|
||
private static void checkReplicationScope(final ColumnFamilyDescriptor cfd) throws IOException { | ||
// check replication scope | ||
WALProtos.ScopeType scop = WALProtos.ScopeType.valueOf(cfd.getScope()); | ||
if (scop == null) { | ||
String message = "Replication scope for column family " + cfd.getNameAsString() + " is " | ||
+ cfd.getScope() + " which is invalid."; | ||
|
||
LOG.error(message); | ||
throw new DoNotRetryIOException(message); | ||
} | ||
} | ||
|
||
private static void checkCompactionPolicy(Configuration conf, TableDescriptor td) | ||
private static void checkReplicationScope(final Configuration conf, final TableDescriptor td) | ||
throws IOException { | ||
// FIFO compaction has some requirements | ||
// Actually FCP ignores periodic major compactions | ||
String className = td.getValue(DefaultStoreEngine.DEFAULT_COMPACTION_POLICY_CLASS_KEY); | ||
if (className == null) { | ||
className = conf.get(DefaultStoreEngine.DEFAULT_COMPACTION_POLICY_CLASS_KEY, | ||
ExploringCompactionPolicy.class.getName()); | ||
} | ||
|
||
int blockingFileCount = HStore.DEFAULT_BLOCKING_STOREFILE_COUNT; | ||
String sv = td.getValue(HStore.BLOCKING_STOREFILES_KEY); | ||
if (sv != null) { | ||
blockingFileCount = Integer.parseInt(sv); | ||
} else { | ||
blockingFileCount = conf.getInt(HStore.BLOCKING_STOREFILES_KEY, blockingFileCount); | ||
} | ||
|
||
for (ColumnFamilyDescriptor hcd : td.getColumnFamilies()) { | ||
String compactionPolicy = | ||
hcd.getConfigurationValue(DefaultStoreEngine.DEFAULT_COMPACTION_POLICY_CLASS_KEY); | ||
if (compactionPolicy == null) { | ||
compactionPolicy = className; | ||
} | ||
if (!compactionPolicy.equals(FIFOCompactionPolicy.class.getName())) { | ||
continue; | ||
// Setting logs to warning instead of throwing exception if sanityChecks are disabled | ||
boolean logWarn = !shouldSanityCheck(conf, td); | ||
try { | ||
for (ColumnFamilyDescriptor cfd : td.getColumnFamilies()) { | ||
// check replication scope | ||
WALProtos.ScopeType scop = WALProtos.ScopeType.valueOf(cfd.getScope()); | ||
if (scop == null) { | ||
String message = "Replication scope for column family " + cfd.getNameAsString() + " is " | ||
+ cfd.getScope() + " which is invalid."; | ||
|
||
throw new DoNotRetryIOException(message); | ||
} | ||
} | ||
// FIFOCompaction | ||
String message = null; | ||
} catch (IOException e) { | ||
warnOrThrowExceptionForFailure(logWarn, e.getMessage(), e); | ||
} | ||
|
||
// 1. Check TTL | ||
if (hcd.getTimeToLive() == ColumnFamilyDescriptorBuilder.DEFAULT_TTL) { | ||
message = "Default TTL is not supported for FIFO compaction"; | ||
throw new IOException(message); | ||
} | ||
} | ||
|
||
// 2. Check min versions | ||
if (hcd.getMinVersions() > 0) { | ||
message = "MIN_VERSION > 0 is not supported for FIFO compaction"; | ||
throw new IOException(message); | ||
private static void checkCompactionPolicy(final Configuration conf, final TableDescriptor td) | ||
throws IOException { | ||
try { | ||
// FIFO compaction has some requirements | ||
// Actually FCP ignores periodic major compactions | ||
String className = td.getValue(DefaultStoreEngine.DEFAULT_COMPACTION_POLICY_CLASS_KEY); | ||
if (className == null) { | ||
className = conf.get(DefaultStoreEngine.DEFAULT_COMPACTION_POLICY_CLASS_KEY, | ||
ExploringCompactionPolicy.class.getName()); | ||
} | ||
|
||
// 3. blocking file count | ||
sv = hcd.getConfigurationValue(HStore.BLOCKING_STOREFILES_KEY); | ||
int blockingFileCount = HStore.DEFAULT_BLOCKING_STOREFILE_COUNT; | ||
String sv = td.getValue(HStore.BLOCKING_STOREFILES_KEY); | ||
if (sv != null) { | ||
blockingFileCount = Integer.parseInt(sv); | ||
} else { | ||
blockingFileCount = conf.getInt(HStore.BLOCKING_STOREFILES_KEY, blockingFileCount); | ||
} | ||
if (blockingFileCount < 1000) { | ||
message = | ||
"Blocking file count '" + HStore.BLOCKING_STOREFILES_KEY + "' " + blockingFileCount | ||
+ " is below recommended minimum of 1000 for column family " + hcd.getNameAsString(); | ||
throw new IOException(message); | ||
|
||
for (ColumnFamilyDescriptor hcd : td.getColumnFamilies()) { | ||
String compactionPolicy = | ||
hcd.getConfigurationValue(DefaultStoreEngine.DEFAULT_COMPACTION_POLICY_CLASS_KEY); | ||
if (compactionPolicy == null) { | ||
compactionPolicy = className; | ||
} | ||
if (!compactionPolicy.equals(FIFOCompactionPolicy.class.getName())) { | ||
continue; | ||
} | ||
// FIFOCompaction | ||
String message = null; | ||
|
||
// 1. Check TTL | ||
if (hcd.getTimeToLive() == ColumnFamilyDescriptorBuilder.DEFAULT_TTL) { | ||
message = "Default TTL is not supported for FIFO compaction"; | ||
throw new IOException(message); | ||
} | ||
|
||
// 2. Check min versions | ||
if (hcd.getMinVersions() > 0) { | ||
message = "MIN_VERSION > 0 is not supported for FIFO compaction"; | ||
throw new IOException(message); | ||
} | ||
|
||
// 3. blocking file count | ||
sv = hcd.getConfigurationValue(HStore.BLOCKING_STOREFILES_KEY); | ||
if (sv != null) { | ||
blockingFileCount = Integer.parseInt(sv); | ||
} | ||
if (blockingFileCount < 1000) { | ||
message = | ||
"Blocking file count '" + HStore.BLOCKING_STOREFILES_KEY + "' " + blockingFileCount | ||
+ " is below recommended minimum of 1000 for column family " + hcd.getNameAsString(); | ||
throw new IOException(message); | ||
} | ||
} | ||
} catch (IOException e) { | ||
warnOrThrowExceptionForFailure(false, e.getMessage(), e); | ||
} | ||
} | ||
|
||
private static void checkBloomFilterType(ColumnFamilyDescriptor cfd) throws IOException { | ||
Configuration conf = new CompoundConfiguration().addStringMap(cfd.getConfiguration()); | ||
private static void checkBloomFilterType(final Configuration conf, final TableDescriptor td) | ||
throws IOException { | ||
// Setting logs to warning instead of throwing exception if sanityChecks are disabled | ||
boolean logWarn = !shouldSanityCheck(conf, td); | ||
try { | ||
BloomFilterUtil.getBloomFilterParam(cfd.getBloomFilterType(), conf); | ||
} catch (IllegalArgumentException e) { | ||
throw new DoNotRetryIOException("Failed to get bloom filter param", e); | ||
for (ColumnFamilyDescriptor cfd : td.getColumnFamilies()) { | ||
Configuration cfdConf = new CompoundConfiguration().addStringMap(cfd.getConfiguration()); | ||
try { | ||
BloomFilterUtil.getBloomFilterParam(cfd.getBloomFilterType(), cfdConf); | ||
} catch (IllegalArgumentException e) { | ||
throw new DoNotRetryIOException("Failed to get bloom filter param", e); | ||
} | ||
} | ||
} catch (IOException e) { | ||
warnOrThrowExceptionForFailure(logWarn, e.getMessage(), e); | ||
} | ||
} | ||
|
||
public static void checkCompression(final TableDescriptor td) throws IOException { | ||
for (ColumnFamilyDescriptor cfd : td.getColumnFamilies()) { | ||
CompressionTest.testCompression(cfd.getCompressionType()); | ||
CompressionTest.testCompression(cfd.getCompactionCompressionType()); | ||
CompressionTest.testCompression(cfd.getMajorCompactionCompressionType()); | ||
CompressionTest.testCompression(cfd.getMinorCompactionCompressionType()); | ||
public static void checkCompression(final Configuration conf, final TableDescriptor td) | ||
throws IOException { | ||
// Setting logs to warning instead of throwing exception if sanityChecks are disabled | ||
boolean logWarn = !shouldSanityCheck(conf, td); | ||
try { | ||
for (ColumnFamilyDescriptor cfd : td.getColumnFamilies()) { | ||
CompressionTest.testCompression(cfd.getCompressionType()); | ||
CompressionTest.testCompression(cfd.getCompactionCompressionType()); | ||
CompressionTest.testCompression(cfd.getMajorCompactionCompressionType()); | ||
CompressionTest.testCompression(cfd.getMinorCompactionCompressionType()); | ||
} | ||
} catch (IOException e) { | ||
warnOrThrowExceptionForFailure(logWarn, e.getMessage(), e); | ||
} | ||
} | ||
|
||
public static void checkEncryption(final Configuration conf, final TableDescriptor td) | ||
throws IOException { | ||
for (ColumnFamilyDescriptor cfd : td.getColumnFamilies()) { | ||
EncryptionTest.testEncryption(conf, cfd.getEncryptionType(), cfd.getEncryptionKey()); | ||
// Setting logs to warning instead of throwing exception if sanityChecks are disabled | ||
boolean logWarn = !shouldSanityCheck(conf, td); | ||
try { | ||
for (ColumnFamilyDescriptor cfd : td.getColumnFamilies()) { | ||
EncryptionTest.testEncryption(conf, cfd.getEncryptionType(), cfd.getEncryptionKey()); | ||
} | ||
} catch (IOException e) { | ||
warnOrThrowExceptionForFailure(logWarn, e.getMessage(), e); | ||
} | ||
} | ||
|
||
public static void checkClassLoading(final Configuration conf, final TableDescriptor td) | ||
throws IOException { | ||
RegionSplitPolicy.getSplitPolicyClass(td, conf); | ||
RegionCoprocessorHost.testTableCoprocessorAttrs(conf, td); | ||
// Setting logs to warning instead of throwing exception if sanityChecks are disabled | ||
boolean logWarn = !shouldSanityCheck(conf, td); | ||
try { | ||
RegionSplitPolicy.getSplitPolicyClass(td, conf); | ||
RegionCoprocessorHost.testTableCoprocessorAttrs(conf, td); | ||
} catch (Exception e) { | ||
warnOrThrowExceptionForFailure(logWarn, e.getMessage(), e); | ||
} | ||
} | ||
|
||
// HBASE-13350 - Helper method to log warning on sanity check failures if checks disabled. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,6 +27,7 @@ | |
import java.util.concurrent.ScheduledExecutorService; | ||
import java.util.concurrent.ThreadPoolExecutor; | ||
import java.util.concurrent.TimeUnit; | ||
import org.apache.hadoop.conf.Configuration; | ||
import org.apache.hadoop.hbase.CompatibilitySingletonFactory; | ||
import org.apache.hadoop.hbase.HBaseClassTestRule; | ||
import org.apache.hadoop.hbase.HBaseTestingUtil; | ||
|
@@ -40,9 +41,11 @@ | |
import org.apache.hadoop.hbase.testclassification.LargeTests; | ||
import org.apache.hadoop.hbase.testclassification.RegionServerTests; | ||
import org.apache.hadoop.hbase.util.EnvironmentEdgeManagerTestHelper; | ||
import org.apache.hadoop.hbase.util.TableDescriptorChecker; | ||
import org.apache.hadoop.metrics2.MetricsExecutor; | ||
import org.junit.AfterClass; | ||
import org.junit.Assert; | ||
import org.junit.BeforeClass; | ||
import org.junit.ClassRule; | ||
import org.junit.Test; | ||
import org.junit.experimental.categories.Category; | ||
|
@@ -60,6 +63,14 @@ public class TestOpenRegionFailedMemoryLeak { | |
|
||
private static HBaseTestingUtil TEST_UTIL = new HBaseTestingUtil(); | ||
|
||
@BeforeClass | ||
public static void startCluster() throws Exception { | ||
Configuration conf = TEST_UTIL.getConfiguration(); | ||
|
||
// Enable sanity check for coprocessor | ||
conf.setBoolean(TableDescriptorChecker.TABLE_SANITY_CHECKS, true); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why we need this change? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, it's due to the fact that this test checks region open is failing due to invalid loaded coprocessor at line 90 below, before this change it was always true (since the coprocessor loading was independent of sanity check). However after this change, we have made it configurable based on the sanity check config (which is always false in hbase-server's test related hbase-site.xml), so we need to explicitly enable sanity check in the test setup There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK, seems to have some side effects. Need to fill a release note. |
||
} | ||
|
||
@AfterClass | ||
public static void tearDown() throws IOException { | ||
EnvironmentEdgeManagerTestHelper.reset(); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this come before the conf check? Otherwise if you enabled sanity checks globally, then I don't think TableDescriptor could be used to override?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see you sort of pulled this logic from lines 71-79 below, but inverted the conditions. But I think the way you have it here is now more restrictive and doesn't allow the table descriptor to override.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, seems true. Have added the table descriptor check first so that it can override the global check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The first line in the sanityCheck method has aleady one the trick for you? It creates a CompoundConfiguration with td.getValues override the configs in Configuration.