kgeisz commented on code in PR #7730:
URL: https://github.com/apache/hbase/pull/7730#discussion_r4288552015
##########
hbase-client/src/main/java/org/apache/hadoop/hbase/client/ConnectionFactory.java:
##########
@@ -595,16 +595,29 @@ public static CompletableFuture<AsyncConnection>
createAsyncConnection(URI conne
future.completeExceptionally(new IOException("clusterid came back
null"));
return;
}
- Class<? extends AsyncConnection> clazz = appliedConf.getClass(
- HBASE_CLIENT_ASYNC_CONNECTION_IMPL, AsyncConnectionImpl.class,
AsyncConnection.class);
- try {
- future.complete(user.runAs((PrivilegedExceptionAction<
- ? extends AsyncConnection>) () ->
ReflectionUtils.newInstance(clazz, appliedConf,
- registry, clusterId, null, user, connectionAttributes)));
- } catch (Exception e) {
- registry.close();
- future.completeExceptionally(e);
- }
+ // Fetch meta table name from registry
+ addListener(registry.getMetaTableName(), (metaTableName, metaError) ->
{
Review 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.
##########
hbase-common/src/main/java/org/apache/hadoop/hbase/CellComparatorImpl.java:
##########
@@ -786,9 +786,21 @@ public static CellComparator getCellComparator(TableName
tableName) {
* @return CellComparator to use going off the {@code tableName} passed.
*/
public static CellComparator getCellComparator(byte[] tableName) {
- // FYI, TableName.toBytes does not create an array; just returns existing
array pointer.
- return Bytes.equals(tableName, TableName.META_TABLE_NAME.toBytes())
+ return isMetaTable(tableName)
? MetaCellComparator.META_COMPARATOR
: CellComparatorImpl.COMPARATOR;
}
+
+ /**
+ * @see TableName#isMetaTableName(TableName) for the canonical definition of
what qualifies as a
+ * meta table. Kept as a {@code byte[]}-friendly adapter so we can
avoid materializing a
+ * {@link TableName} on hot comparator-selection paths when the input
is malformed.
+ */
+ static boolean isMetaTable(byte[] tableName) {
+ try {
+ return TableName.isMetaTableName(TableName.valueOf(tableName));
+ } catch (IllegalArgumentException e) {
+ return false;
+ }
+ }
Review 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.
```suggestion
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;
}
}
```
##########
hbase-common/src/main/java/org/apache/hadoop/hbase/TableName.java:
##########
@@ -123,9 +130,32 @@ public static TableName
initializeHbaseMetaTableName(Configuration conf) {
/** One globally disallowed name */
public static final String DISALLOWED_TABLE_NAME = "zookeeper";
- /** Returns True if <code>tn</code> is the hbase:meta table name. */
+ /**
+ * Pattern that the qualifier of a meta table must match. Either the literal
{@code meta} (the
+ * default meta table) or {@code meta_<suffix>} where {@code <suffix>} is
one or more lowercase
+ * alphanumeric characters (the form used for cluster-specific meta tables,
e.g. read replica meta
+ * tables). This is the single source of truth for what qualifies as a meta
table name; all
+ * 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]+)?");
Review Comment:
nit: Might be worth adding a unit test for this
```
# Invalid
meta_
meta_SUFFIX
meta__suffix
metatable
etc.
```
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/HBaseServerBase.java:
##########
@@ -745,4 +745,16 @@ private String resolveHostName(Configuration conf, String
useThisHostnameInstead
protected abstract boolean cacheTableDescriptor();
protected abstract boolean clusterMode();
+
+ protected TableName getDefaultMetaTableName() {
+ return TableName.META_TABLE_NAME;
+ }
+
+ @Override
+ public TableName getMetaTableName() {
+ // 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();
Review Comment:
I'm also wondering this
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/util/FSTableDescriptors.java:
##########
@@ -143,24 +143,34 @@ public static void
tryUpdateMetaTableDescriptor(Configuration conf) throws IOExc
CommonFSUtils.getRootDir(conf));
}
+ private static TableName getMetaTableNameFromConf(Configuration conf) {
+ // TODO: Support cluster-specific (e.g. read-replica) meta table names.
This method is static
+ // and runs from contexts (FS bootstrap) where neither MasterRegion nor a
Connection is yet
+ // 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;
Review Comment:
Can this method be updated now that Read-Replica has been merged, or do you
plan on doing is in another PR?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
