-
Notifications
You must be signed in to change notification settings - Fork 3.4k
HBASE-29691: Change TableName.META_TABLE_NAME from being a global static #7730
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
base: master
Are you sure you want to change the base?
Conversation
Change TableName.META_TABLE_NAME from a global static constant to a dynamically discovered value from ConnectionRegistry. Key changes: - Add ConnectionRegistry.getMetaTableName() method for dynamic discovery - Add MetaTableNameStore for persisting meta table name in master region - Update TableName to support dynamic meta table name - Update HMaster to integrate with MetaTableNameStore - Update all client and server code to use dynamic meta table name - Add protobuf changes for meta table name in Registry.proto Refactoring for the test classes to be handled separately.
anmolnar
left a comment
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. What are the build failures?
kgeisz
left a comment
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 overall. I have some nit comments below. Also, I see PR #7558 is still open. Should that PR be closed?
| // TODO: How come Meta regions still do not have encoded region names? Fix. | ||
| // hbase:meta,,1.1588230740 should be the hbase:meta first region name. | ||
| // TODO: For now, hardcode to default. Future: lazy initialization based on config or make it use | ||
| // conenction |
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.
nit:
| // conenction | |
| // connection |
| @@ -787,8 +787,13 @@ public static CellComparator getCellComparator(TableName tableName) { | |||
| */ | |||
| public static CellComparator getCellComparator(byte[] tableName) { | |||
| // FYI, TableName.toBytes does not create an array; just returns existing array pointer. | |||
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.
nit: Is this comment still applicable?
| if (!createMetaEntriesFailures.isEmpty()) { | ||
| LOG.warn( | ||
| "Failed to create entries in hbase:meta for {}/{} RegionInfo descriptors. First" | ||
| "Failed to create entries in {}} for {}/{} RegionInfo descriptors. First" |
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.
nit
| "Failed to create entries in {}} for {}/{} RegionInfo descriptors. First" | |
| "Failed to create entries in {} for {}/{} RegionInfo descriptors. First" |
| if (!cfs.contains(family)) { | ||
| throw new HBaseIOException( | ||
| "Delete of hbase:meta column family " + Bytes.toString(family)); | ||
| "Delete of " + env.getMetaTableName() + " column family " + Bytes.toString(family)); |
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.
nit: IMO this could be a little more clear
| "Delete of " + env.getMetaTableName() + " column family " + Bytes.toString(family)); | |
| "Invalid deletion of " + env.getMetaTableName() + " column family: " + Bytes.toString(family)); |
| LOG.debug("Loaded meta table name from Master Local Region: {}", cachedMetaTableName); | ||
| return cachedMetaTableName; | ||
| } | ||
| LOG.info("No stored meta table name found in Master Local Region: {}", cachedMetaTableName); |
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.
nit
| LOG.info("No stored meta table name found in Master Local Region: {}", cachedMetaTableName); | |
| LOG.info("No stored meta table name found in Master Local Region: {}", cachedMetaTableName); |
| startEndKeys.get(idx + 1).getFirst()) == 0) | ||
| ) { | ||
| throw new IOException("The endkey of one region for table " + tableName | ||
| + " is not equal to the startkey of the next region in hbase:meta." |
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.
And here
| LOG.warn( | ||
| "Unable to close region " + hi.getRegionNameAsString() | ||
| + " because {} had invalid or missing " + HConstants.CATALOG_FAMILY_STR + ":" | ||
| + Bytes.toString(HConstants.REGIONINFO_QUALIFIER) + " qualifier value.", | ||
| connection.getMetaTableName()); |
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.
nit: I see a mixture of using string concatenation as well as {} in the LOG.warn() method. IMO is should consistently use the {} convention.
| LOG.info("Patching {} with .regioninfo: " + hbi.getHdfsHRI(), | ||
| connection.getMetaTableName()); |
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.
nit: Mixture of string concatenation and {} in log method
| LOG.info("Patching {} with with .regioninfo: " + hbi.getHdfsHRI(), | ||
| connection.getMetaTableName()); |
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.
nit: Mixture of string concatenation and {} in log method
| + hbi.getMetaEntry().regionServer + " but is multiply assigned to region servers " | ||
| + Joiner.on(", ").join(hbi.getDeployedOn())); | ||
| "Region " + descriptiveName + " is listed in " + connection.getMetaTableName() | ||
| + " on region server " + hbi.getMetaEntry().regionServer + " but is multiply assigned" |
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.
nit: Do you have any idea what they mean by "multiply assigned"? Like "multi-assigned"?
HBASE-29691: Change TableName.META_TABLE_NAME from being a global static
Change TableName.META_TABLE_NAME from a global static constant to a dynamically discovered value from ConnectionRegistry.
Please refer to earlier discussion on the PR #7558 for further details.
Key changes:
Refactoring for the test classes to be handled separately.