-
Notifications
You must be signed in to change notification settings - Fork 3.4k
HBASE-22624 Should sanity check table configuration when clone snapsh… #335
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
Conversation
💔 -1 overall
This message was automatically generated. |
@@ -94,7 +95,7 @@ public void setConf(Configuration conf) { | |||
String.format("%s.%s", TEST_NAME, LoadTestTool.OPT_COLUMN_FAMILIES), | |||
StringUtils.join(",", DEFAULT_COLUMN_FAMILIES)); | |||
|
|||
conf.setBoolean("hbase.table.sanity.checks", true); | |||
conf.setBoolean(TableDescriptorChecker.TABLE_SANITY_CHECKS, true); |
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.
Is this different from other conf.setBoolean("hbase.table.sanity.checks", true)
which is removed from other tests like TestAvoidCellReferencesIntoShippedBlocks based on the default value of TableDescriptorChecker.TABLE_SANITY_CHECKS?
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.
This is in moudle hbase-it. And this may be runed by user directly and with user's hbase-site.xml? So i didn't remove this.
*/ | ||
public static void sanityCheck(final Configuration conf, final TableDescriptor td, | ||
boolean checkCompression, boolean checkEncryption) throws IOException { | ||
boolean logWarn = false; |
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.
Can we add a comment here along the lines of: "Setting this to true logs the warning instead of throwing"?
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.
OK.
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java
Outdated
Show resolved
Hide resolved
hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java
Outdated
Show resolved
Hide resolved
…ot to a new table
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.
LGTM, +1 if HBASE QA is OK.
💔 -1 overall
This message was automatically generated. |
The failed ut should not related. Let me take a try on my local PC. |
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.
lgtm
…ot to a new table