Uh oh!
There was an error while loading. Please reload this page.
HBASE-25368 Filter out more invalid encoded name in isEncodedRegionNa… - #2753
Merged
Conversation
Apache-HBase
commented
Dec 8, 2020
🎊 +1 overall
This message was automatically generated. |
| if (regionName.length > MD5_HEX_LENGTH) { | ||
| return false; | ||
| } else if (regionName.length == MD5_HEX_LENGTH) { | ||
| return true; |
Contributor
There was a problem hiding this comment.
What are we protecting against? Could we be passed a tablename? If so, why can't we have a tablename that is an MD5? Or 32 bytes in size? Should we check it is all hex at least? I suppose if someone passes a tablename that is an md5, then they are asking for trouble?
ContributorAuthor
There was a problem hiding this comment.
Thanks @saintstack. Two issues are addressed.
For compact/flush/split with the tableName,
` # Requests a table or region or column family major compaction
def major_compact(table_or_region_name, family = nil, type = 'NORMAL')
family_bytes = nil
family_bytes = family.to_java_bytes unless family.nil?
compact_type = nil
if type == 'NORMAL'
compact_type = org.apache.hadoop.hbase.client.CompactType::NORMAL
elsif type == 'MOB'
compact_type = org.apache.hadoop.hbase.client.CompactType::MOB
else
raise ArgumentError, 'only NORMAL or MOB accepted for type!'
end
begin
if family_bytes.nil?
@admin.majorCompactRegion(table_or_region_name.to_java_bytes)
else
@admin.majorCompactRegion(table_or_region_name.to_java_bytes, family_bytes)
end
rescue java.lang.IllegalArgumentException, org.apache.hadoop.hbase.UnknownRegionException
if family_bytes.nil?
@admin.majorCompact(TableName.valueOf(table_or_region_name), compact_type)
else
@admin.majorCompact(TableName.valueOf(table_or_region_name), family_bytes, compact_type)
end
end
end
`
it first calls majorCompactRegion() with tableName as input. It expects an IllegalArgumentException or UnknownRegionException to call majorCompact().
This normally involves a registry query to get this UnknownRegionException, this is an expensive path.
If it knows that the input string is not an encodedRegionName or regionName, it can throw out IllegalArgumentException without registry query.
For the case that a md5 hex being used as tableName, it has to go through expensive path to find out that this is not an encodedRegionName, it will still work, just optimization wont be applied in this case.
It also fixes a bug for a table name length over 32 bytes, currently, compact/flush/split this tableName from shell will fail.
Contributor
There was a problem hiding this comment.
THanks for explanation. Shove this up in the JIRA description? Its good. Thanks.
| try { | ||
| Integer.parseInt(encodedName); | ||
| // If this is a valid integer, it could be hbase:meta's encoded region name. | ||
| return true; |
Contributor
There was a problem hiding this comment.
We know the hbase:meta int. It does not change. Compare it? When meta splits, it will have md5 for new regions.
ContributorAuthor
There was a problem hiding this comment.
It needs to have meta regionname to compare (another query). This will defeat the purpose of optimization, so instead, it just check if it is an integer. If it is an integer, then this is a possible encoded region name.
Uh oh!
There was an error while loading. Please reload this page.
Apache-HBase
commented
Dec 8, 2020
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Dec 8, 2020
💔 -1 overall
This message was automatically generated. |
huaxiangsun
commented
Dec 9, 2020
| ADMIN.flush(tableName); | ||
| //ADMIN.majorCompactRegion(Bytes.toBytes("abcdefghijklmnopqrstuvwxyzabcdedgeghijklmnopqrs")); | ||
| ADMIN.majorCompactRegion(Bytes.toBytes("abcd")); |
ContributorAuthor
There was a problem hiding this comment.
Will remove these two lines, they are used for testing before the new test case.
saintstack
commented
Dec 10, 2020
Contributor
Failure seems unrelated. Rerunning anyways while Huaxiang sleeps. |
saintstack
approved these changes
Dec 10, 2020
saintstack
left a comment
Contributor
There was a problem hiding this comment.
LGTM. Rerunning tests to make sure the failure unrelated.
Apache-HBase
commented
Dec 10, 2020
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Dec 10, 2020
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Dec 10, 2020
🎊 +1 overall
This message was automatically generated. |
…me(byte[] regionName)
huaxiangsunforce-pushed
the
master.HBASE-25368
branch
from
December 10, 2020 17:36
d4167a7 to
2bf1fdeCompareApache-HBase
commented
Dec 10, 2020
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Dec 10, 2020
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Dec 10, 2020
🎊 +1 overall
This message was automatically generated. |
saintstack
commented
Dec 11, 2020
Contributor
Seems good to go @huaxiangsun ? |
huaxiangsun
commented
Dec 11, 2020
ContributorAuthor
Let me introduce the isMD5Hash, @saintstack |
huaxiangsun
commented
Dec 12, 2020
ContributorAuthor
Actually, I thought about it. If I introduce apache.common.codec.MD5Hex.decodeHex(), it is heavy (allocate/copy). I think it is ok without introducing MD5 as most of the calls are the real encodedRegionNames. Only for shell commends, table name can be passed in. In this case, it is ok to have a query to registry in case table name length is 32 bytes (which could be rare as well). |
saintstack
commented
Dec 14, 2020
Contributor
Ok. Still +1 on this @huaxiangsun |
This file contains hidden or 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
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
…me(byte[] regionName)
This addresses a few issues:
Signed-off-by: stack stack@apache.org