HBASE-25922 - Disabled sanity checks ignored on snapshot restore #4533
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@virajjasani Can you help in reviewing this ? |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
@ujjawal4046 are these failed tests passing in your local? https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4533/1/testReport/ |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
…ading check fails while opening hregion TestAssignmentManagerMetrics - Enable sanity checks on RS so that region reopen fails during modify table, disable sanity check on HMaster so that modify table doesn't terminate prematurely without calling assigment manager
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
| if (conf.getBoolean(TABLE_SANITY_CHECKS, DEFAULT_TABLE_SANITY_CHECKS)) { | ||
| return true; | ||
| } | ||
| String tableVal = td.getValue(TABLE_SANITY_CHECKS); |
There was a problem hiding this comment.
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.
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.
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.
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.
| @@ -186,9 +178,18 @@ public static void sanityCheck(final Configuration c, final TableDescriptor td) | |||
| } | |||
|
|
|||
| // check replication scope | |||
| checkReplicationScope(hcd); | |||
| try { | |||
There was a problem hiding this comment.
it would be nice to keep these check methods somewhat consistent. You removed the warnOrThrowExceptionForFailure from the above checks, but added them here. Can you instead keep it consistent (all here or all in the check methods)?
That might mean either removing this diff here, or adding back the try/catches above.
There was a problem hiding this comment.
Sure, to keep consistency for all additional check methods I have moved the warnOrThrowExceptionForFailure logic there.
i) Table Descriptor should come before global check to allow for overrides ii) Check methods in TableDescriptorChecker can do sanity check on their own
ujjawal4046
commented
Jun 26, 2022
There was a problem hiding this comment.
Thanks for the review @bbeaudreault. Have handled your comments.
| @@ -186,9 +178,18 @@ public static void sanityCheck(final Configuration c, final TableDescriptor td) | |||
| } | |||
|
|
|||
| // check replication scope | |||
| checkReplicationScope(hcd); | |||
| try { | |||
There was a problem hiding this comment.
Sure, to keep consistency for all additional check methods I have moved the warnOrThrowExceptionForFailure logic there.
| if (conf.getBoolean(TABLE_SANITY_CHECKS, DEFAULT_TABLE_SANITY_CHECKS)) { | ||
| return true; | ||
| } | ||
| String tableVal = td.getValue(TABLE_SANITY_CHECKS); |
There was a problem hiding this comment.
Hmm, seems true. Have added the table descriptor check first so that it can override the global check
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
Apache9
reviewed
Jul 11, 2022
| if (conf.getBoolean(TABLE_SANITY_CHECKS, DEFAULT_TABLE_SANITY_CHECKS)) { | ||
| return true; | ||
| } | ||
| String tableVal = td.getValue(TABLE_SANITY_CHECKS); |
There was a problem hiding this comment.
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.
added 2 commits
July 31, 2022 20:07
The issue here is HRegion code calls various public methods ( |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
Apache9
reviewed
Aug 21, 2022
There was a problem hiding this comment.
No big concerns then. @bbeaudreault Do you have any other comments here?
| @@ -185,11 +175,6 @@ public static void sanityCheck(final Configuration c, final TableDescriptor td) | |||
| warnOrThrowExceptionForFailure(logWarn, message, null); | |||
There was a problem hiding this comment.
This is the place where we use logWarn in this method? Then let's move the boolean logWarn = !shouldSanityCheck(conf); line here?
There was a problem hiding this comment.
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.
Ah, OK, in unchanged code.
| Configuration conf = TEST_UTIL.getConfiguration(); | ||
|
|
||
| // Enable sanity check for coprocessor | ||
| conf.setBoolean(TableDescriptorChecker.TABLE_SANITY_CHECKS, true); |
There was a problem hiding this comment.
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.
OK, seems to have some side effects. Need to fill a release note.
ujjawal4046
commented
Aug 21, 2022
There was a problem hiding this comment.
@Apache9 Have replied to your queries, can you take a look ?
| @@ -185,11 +175,6 @@ public static void sanityCheck(final Configuration c, final TableDescriptor td) | |||
| warnOrThrowExceptionForFailure(logWarn, message, null); | |||
There was a problem hiding this comment.
There are multiple checks based on logWarn variable before this line (for e.g. at line no 92, 106, 127, 134 above)
| Configuration conf = TEST_UTIL.getConfiguration(); | ||
|
|
||
| // Enable sanity check for coprocessor | ||
| conf.setBoolean(TableDescriptorChecker.TABLE_SANITY_CHECKS, true); |
There was a problem hiding this comment.
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
Apache9
approved these changes
Aug 22, 2022
bbeaudreault
approved these changes
Aug 24, 2022
bbeaudreault
pushed a commit
that referenced
this pull request
Aug 26, 2022
Co-authored-by: Ujjawal Kumar <u.kumar@ukumar-ltmit1s.internal.salesforce.com> Signed-off-by: Duo Zhang <zhangduo@apache.org> Signed-off-by: Bryan Beaudreault <bbeaudreault@apache.org> Conflicts: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
bbeaudreault
pushed a commit
that referenced
this pull request
Aug 26, 2022
Co-authored-by: Ujjawal Kumar <u.kumar@ukumar-ltmit1s.internal.salesforce.com> Signed-off-by: Duo Zhang <zhangduo@apache.org> Signed-off-by: Bryan Beaudreault <bbeaudreault@apache.org> Conflicts: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
ujjawal4046
added a commit
to ujjawal4046/hbase
that referenced
this pull request
Aug 26, 2022
…che#4533) Signed-off-by: Duo Zhang <zhangduo@apache.org> Signed-off-by: Bryan Beaudreault <bbeaudreault@apache.org> Conflicts: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
bbeaudreault
pushed a commit
that referenced
this pull request
Aug 27, 2022
…) (#4734) Signed-off-by: Duo Zhang <zhangduo@apache.org> Signed-off-by: Bryan Beaudreault <bbeaudreault@apache.org> Conflicts: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
vinayakphegde
pushed a commit
to vinayakphegde/hbase
that referenced
this pull request
Apr 4, 2024
…che#4533) (apache#4734) Signed-off-by: Duo Zhang <zhangduo@apache.org> Signed-off-by: Bryan Beaudreault <bbeaudreault@apache.org> Conflicts: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java (cherry picked from commit 8659c93) Change-Id: I13ded9ca65b094a0cf05920e42a44fdfc4381a56
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
JIRA - https://issues.apache.org/jira/browse/HBASE-25922
TableDescriptorChecker.checkClassLoading()while trying to restore a snapshot. This should adhere to the sanity checks provided as part of TableDescriptorChecker.