Repository navigation
Conversation
anmolnar
left a comment
There was a problem hiding this comment.
lgtm. What are the build failures?
| + 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.
nit: Do you have any idea what they mean by "multiply assigned"? Like "multi-assigned"?
|
Just came back from vacation... |
There was a problem hiding this comment.
Pull request overview
This pull request implements HBASE-29691, which changes TableName.META_TABLE_NAME from a global static constant to a dynamically discovered value. The change enables support for cluster-specific meta table names, particularly for read replica clusters and other specialized configurations.
Changes:
- Adds
ConnectionRegistry.getMetaTableName()method for dynamic meta table name discovery - Introduces
MetaTableNameStoreclass to persist the meta table name in the Master Local Region - Updates
Connection,AsyncConnection, andMasterServicesinterfaces to exposegetMetaTableName()method - Modifies
TableName.isMetaTableName()to support both default "hbase:meta" and variant table names with suffixes - Updates all client and server code paths to use the dynamic meta table name instead of the static constant
- Adds new RPC endpoint
GetMetaTableNamein Registry.proto for retrieving the meta table name from the master
Reviewed changes
Copilot reviewed 98 out of 98 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| hbase-protocol-shaded/src/main/protobuf/server/Registry.proto | Adds GetMetaTableName RPC request/response messages |
| hbase-server/src/main/java/org/apache/hadoop/hbase/master/MetaTableNameStore.java | New class to store/retrieve meta table name from Master Local Region |
| hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java | Integrates MetaTableNameStore and overrides getMetaTableName() |
| hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java | Implements getMetaTableName() to fetch from master via connection |
| hbase-client/src/main/java/org/apache/hadoop/hbase/client/Connection.java | Adds getMetaTableName() and getMetaTable() helper methods |
| hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncConnection.java | Adds getMetaTableName() and getMetaTable() helper methods |
| hbase-client/src/main/java/org/apache/hadoop/hbase/client/ConnectionRegistry.java | Adds getMetaTableName() method for discovering meta table name |
| hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncConnectionImpl.java | Stores and exposes meta table name fetched from registry |
| hbase-client/src/main/java/org/apache/hadoop/hbase/client/ConnectionFactory.java | Fetches meta table name from registry during connection creation |
| hbase-common/src/main/java/org/apache/hadoop/hbase/TableName.java | Updates isMetaTableName() to support meta table name variants, deprecates META_TABLE_NAME |
| hbase-common/src/main/java/org/apache/hadoop/hbase/CellComparatorImpl.java | Updates comparator selection to support dynamic meta table names |
| hbase-server/src/main/java/org/apache/hadoop/hbase/HBaseServerBase.java | Provides default getMetaTableName() implementation |
| hbase-server/src/main/java/org/apache/hadoop/hbase/HBaseRpcServicesBase.java | Implements getMetaTableName() RPC handler |
| Multiple server-side utilities | Updates HBaseFsck, RegionMover, MetaTableAccessor, and other utilities to use dynamic meta table name |
| Multiple test files | Updates test infrastructure and mocks to support dynamic meta table name |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| LOG.info( | ||
| "The {} config is {} while the replica count in TableDescriptor is {}" | ||
| + " for hbase:meta, altering...", | ||
| HConstants.META_REPLICAS_NUM, replicasNumInConf, metaDesc.getRegionReplication()); | ||
| HConstants.META_REPLICAS_NUM, replicasNumInConf, metaDesc.getRegionReplication(), | ||
| getMetaTableName()); |
There was a problem hiding this comment.
The log format at line 1207 has incorrect parameter count. The format string has 3 placeholders ("The {} config is {} while the replica count in TableDescriptor is {} for hbase:meta, altering...") but 4 arguments are provided: HConstants.META_REPLICAS_NUM, replicasNumInConf, metaDesc.getRegionReplication(), and getMetaTableName(). This will cause a logging error. The fourth parameter getMetaTableName() should be part of the format string template, not an extra argument.
| } catch (IOException e) { | ||
| LOG.info("Exception loading/storing meta table name from master region"); | ||
| throw new RuntimeException(e); | ||
| } |
There was a problem hiding this comment.
The error message at line 1689 is missing important context. When logging an exception, it should describe what operation failed. The message "Exception loading/storing meta table name from master region" should be logged with the exception using LOG.error() or LOG.warn() with the exception as the second parameter, not LOG.info(). Also, rethrowing as RuntimeException loses the original exception message - consider using new RuntimeException("Exception loading/storing meta table name from master region", e) to preserve the exception chain.
| } | ||
|
|
||
| static boolean isMetaTable(byte[] tableName) { | ||
| return Bytes.startsWith(tableName, TableName.META_TABLE_NAME.getName()); |
There was a problem hiding this comment.
The implementation uses TableName.META_TABLE_NAME.getName() as a prefix check, but this could match unintended table names. For example, if someone creates a table "hbase:metadata", this check would incorrectly return true because it starts with "hbase:meta". The logic should verify exact match or use a more precise delimiter-based check. Consider checking if the bytes are exactly equal OR if they match the pattern with an underscore separator.
| return Bytes.startsWith(tableName, TableName.META_TABLE_NAME.getName()); | |
| byte[] metaName = TableName.META_TABLE_NAME.getName(); | |
| // Match exactly "hbase:meta" or names starting with "hbase:meta_" (e.g., "hbase:meta_1"). | |
| if (Bytes.equals(tableName, metaName)) { | |
| return true; | |
| } | |
| if (tableName == null || tableName.length <= metaName.length) { | |
| return false; | |
| } | |
| if (!Bytes.startsWith(tableName, metaName)) { | |
| return false; | |
| } | |
| // Require an underscore delimiter after "hbase:meta" to avoid matching names like "hbase:metadata". | |
| return tableName[metaName.length] == (byte) '_'; |
There was a problem hiding this comment.
You might want to be more specific here.
- Table name equals to
hbase:meta, or - Table name matches pattern
hbase:meta_[a-z0-9]+
As long as we agree on that meta table name will be generated from a hash value.
| private static TableName getMetaTableNameFromConf(Configuration conf) { | ||
| // TODO: Support replica-specific meta table names from masterRegion | ||
| return TableName.META_TABLE_NAME; |
There was a problem hiding this comment.
The TODO comment on line 147 mentions "Support replica-specific meta table names from masterRegion" but this contradicts the PR's design. The PR adds MetaTableNameStore which stores the meta table name in the master region. This method, however, is static and doesn't have access to the master region. The comment and implementation should be aligned - either this method should not exist (and callers should use the dynamic meta table name from the connection/master), or the TODO should explain how static methods will interact with the dynamic meta table name system.
| @Override | ||
| public TableName getMetaTableName() { | ||
| return toAsyncConnection().getMetaTableName(); | ||
| } |
There was a problem hiding this comment.
Calling toAsyncConnection().getMetaTableName() on line 374 will throw NotImplementedException because toAsyncConnection() on line 379 throws NotImplementedException. This creates a method that always throws an exception, which is a bug. The getMetaTableName() implementation should either throw NotImplementedException directly or have a proper implementation that doesn't depend on toAsyncConnection().
| public TableName getMetaTableName() { | ||
| if (asyncClusterConnection != null) { | ||
| try { | ||
| return asyncClusterConnection.getMetaTableName(); | ||
| } catch (Exception e) { | ||
| LOG.warn("Failed to get meta table name from Master", e); | ||
| } | ||
| } | ||
| // Bootstrap | ||
| return super.getMetaTableName(); | ||
| } |
There was a problem hiding this comment.
The getMetaTableName() method in HRegionServer could return different values over time, which could cause inconsistencies. During bootstrap (before asyncClusterConnection is set), it returns the default meta table name from the parent class. After the connection is established, it returns the value from the connection. If there's any caching or state that depends on this value, changing it mid-execution could cause issues. Consider caching the value once it's successfully retrieved from the connection to ensure consistency.
|
Regarding Copilot's concerns about incorrect meta table name prefix matching: is the user able to create tables in the |
f5de4ac to
82ca864
Compare
82ca864 to
533178a
Compare
| // TODO: For now, hardcode to default. Future: lazy initialization based on config or make it use | ||
| // connection |
There was a problem hiding this comment.
Is this going to work for now with the default name?
| @Test | ||
| public void testCreateMetaTable() { | ||
| conn.getAdmin().createTable(TableDescriptorBuilder.newBuilder(TableName.META_TABLE_NAME) | ||
| conn.getAdmin().createTable(TableDescriptorBuilder.newBuilder(conn.getMetaTableName()) |
There was a problem hiding this comment.
I thought that we don't change the default meta table name in unit tests in this patch for now. Rather we'll do them at once if needed. Why is this change?
| @Test | ||
| public void testClearCacheTableName() { | ||
| conn.getLocator().clearCache(TableName.META_TABLE_NAME); | ||
| conn.getLocator().clearCache(conn.getMetaTableName()); |
There was a problem hiding this comment.
Same here. Are these tests not passing without the modification?
| } | ||
|
|
||
| static boolean isMetaTable(byte[] tableName) { | ||
| return Bytes.startsWith(tableName, TableName.META_TABLE_NAME.getName()); |
There was a problem hiding this comment.
You might want to be more specific here.
- Table name equals to
hbase:meta, or - Table name matches pattern
hbase:meta_[a-z0-9]+
As long as we agree on that meta table name will be generated from a hash value.
| public static boolean isMetaTableName(final TableName tn) { | ||
| return tn.equals(TableName.META_TABLE_NAME); | ||
| if ( | ||
| tn == null || !tn.getNamespaceAsString().equals(NamespaceDescriptor.SYSTEM_NAMESPACE_NAME_STR) | ||
| ) { | ||
| return false; | ||
| } | ||
| String qualifier = tn.getQualifierAsString(); | ||
| return qualifier.equals("meta") || qualifier.startsWith("meta_"); | ||
| } |
There was a problem hiding this comment.
Can we do this validation at a single place potentially here in this class?
| @Override | ||
| public TableName getTableName() { | ||
| // Always returns default "hbase:meta" for now | ||
| // Future: will support replica-specific names (e.g., "hbase:meta_replica1") |
There was a problem hiding this comment.
So, this will be the point where we'll populate the meta table name from config.
14dc5f9 to
d0406d6
Compare
da6ed3d to
cd2390c
Compare
cd2390c to
71f7824
Compare
anmolnar
left a comment
There was a problem hiding this comment.
lgtm. A few of my previous questions haven't been answered yet.
| // For now, it is hbase:meta because we don't support custom meta table name. | ||
| // After adding support for custom meta table names, we can calculate this from conf and use it | ||
| // downstream to persist it in Master Region. | ||
| return getDefaultMetaTableName(); |
There was a problem hiding this comment.
Is this comment still valid?
4b14bc6 to
76fd869
Compare
kgeisz
left a comment
There was a problem hiding this comment.
A few comments after doing another review
| static boolean isMetaTable(byte[] tableName) { | ||
| try { | ||
| return TableName.isMetaTableName(TableName.valueOf(tableName)); | ||
| } catch (IllegalArgumentException e) { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
From Claude:
Replace the "TableName.valueOf() + regex" approach with a fast-path byte comparison against the default hbase:meta bytes, falling back to the general check only for non-matching names.
| static boolean isMetaTable(byte[] tableName) { | |
| try { | |
| return TableName.isMetaTableName(TableName.valueOf(tableName)); | |
| } catch (IllegalArgumentException e) { | |
| return false; | |
| } | |
| } | |
| static boolean isMetaTable(byte[] tableName) { | |
| if (Bytes.equals(tableName, TableName.META_TABLE_NAME.toBytes())) { | |
| return true; | |
| } | |
| try { | |
| return TableName.isMetaTableName(TableName.valueOf(tableName)); | |
| } catch (IllegalArgumentException e) { | |
| return false; | |
| } | |
| } |
| future.completeExceptionally(e); | ||
| } | ||
| // Fetch meta table name from registry | ||
| addListener(registry.getMetaTableName(), (metaTableName, metaError) -> { |
There was a problem hiding this comment.
From Claude:
registry.getMetaTableName() is nested inside the getClusterId() callback. These two RPCs are independent. Use CompletableFuture.allOf() to issue both concurrently, saving one RTT on every connection creation. This matters for short-lived connections in tests and tools.
| * callers should go through {@link #isMetaTableName(TableName)} rather than reproducing the | ||
| * check. | ||
| */ | ||
| private static final Pattern META_QUALIFIER_PATTERN = Pattern.compile("meta(_[a-z0-9]+)?"); |
There was a problem hiding this comment.
nit: Might be worth adding a unit test for this
# Invalid
meta_
meta_SUFFIX
meta__suffix
metatable
etc.
| // available, so the dynamic name cannot be resolved here today. A follow-up should plumb the | ||
| // resolved meta TableName from the caller (e.g. via Configuration once the master has written | ||
| // it, or by passing a TableName argument from contexts that know it). | ||
| return TableName.META_TABLE_NAME; |
There was a problem hiding this comment.
Can this method be updated now that Read-Replica has been merged, or do you plan on doing is in another PR?
| // For now, it is hbase:meta because we don't support custom meta table name. | ||
| // After adding support for custom meta table names, we can calculate this from conf and use it | ||
| // downstream to persist it in Master Region. | ||
| return getDefaultMetaTableName(); |
I went through the full diff at Likely must-fix1. Client/server and cross-cluster compatibility
A fallback to 2. Public API compatibility 3. Suffix pattern mismatch Design questions4. Storage location. 5. "A meta table" vs. "this cluster's meta table".
In a shared-storage read-replica setup these become real bugs. Comparing against the resolved name at those sites would separate the two meanings. 6. Stored vs. configured name. The master persists the name on first boot, but several paths still use the static constant ( 7. Backup masters. Performance
Minor
TestsNo new tests are included. Useful additions would be:
|
…rom ConnectionRegistry This is a subtask for the umbrella ticket: HBASE-29691 Change TableName.META_TABLE_NAME from being a global static 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
- Keep GetMetaTableName compatible with older servers and default the Connection/AsyncConnection getMetaTableName() methods - Fix suffix regex handling in TableName and avoid per-call regex work in CellComparatorImpl.isMetaTable - ProtobufUtil.toRegionInfo no longer maps every hbase:meta_* name to FIRST_META_REGIONINFO - Distinguish "any meta table" from "this cluster's meta table" in HMaster and MetaTableAccessor; reconcile stored vs configured name and handle backup masters - Store the meta table name in the master region's state family instead of the info (catalog) family, and drop redundant caching - Use AsyncConnection consistently in RegionStateStore; resolve the meta name dynamically in FSTableDescriptors/InitMetaProcedure - Remove ThriftConnection.getMetaTableName override that always threw - Log/comment cleanups; add TestMetaTableNameStore and extra TableName and registry tests Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
76fd869 to
1f36fe2
Compare
HBASE-29881: 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.