Copilot commented on code in PR #7730:
URL: https://github.com/apache/hbase/pull/7730#discussion_r2841516504
##########
hbase-common/src/main/java/org/apache/hadoop/hbase/CellComparatorImpl.java:
##########
@@ -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.
- return Bytes.equals(tableName, TableName.META_TABLE_NAME.toBytes())
+ // Check if this is a meta table (hbase:meta or hbase:meta_*)
+ return isMetaTable(tableName)
? MetaCellComparator.META_COMPARATOR
: CellComparatorImpl.COMPARATOR;
}
+
+ static boolean isMetaTable(byte[] tableName) {
+ return Bytes.startsWith(tableName, TableName.META_TABLE_NAME.getName());
Review 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.
```suggestion
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) '_';
```
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java:
##########
@@ -1193,7 +1204,8 @@ private void finishActiveMasterInitialization() throws
IOException, InterruptedE
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());
Review 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.
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/master/MetaTableNameStore.java:
##########
@@ -0,0 +1,93 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.master;
+
+import java.io.IOException;
+import org.apache.hadoop.hbase.TableName;
+import org.apache.hadoop.hbase.client.Get;
+import org.apache.hadoop.hbase.client.Put;
+import org.apache.hadoop.hbase.client.Result;
+import org.apache.hadoop.hbase.master.region.MasterRegion;
+import org.apache.hadoop.hbase.util.Bytes;
+import org.apache.yetus.audience.InterfaceAudience;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Stores and retrieves the meta table name for this cluster in the Master
Local Region. This
+ * provides cluster-specific storage for the meta table name.
+ */
[email protected]
+public class MetaTableNameStore {
+ private static final Logger LOG =
LoggerFactory.getLogger(MetaTableNameStore.class);
+ private static final byte[] META_TABLE_NAME_ROW =
Bytes.toBytes("meta_table_name");
+ private static final byte[] INFO_FAMILY = Bytes.toBytes("info");
+ private static final byte[] NAME_QUALIFIER = Bytes.toBytes("name");
+
+ private final MasterRegion masterRegion;
+ private volatile TableName cachedMetaTableName;
+
+ public MetaTableNameStore(MasterRegion masterRegion) {
+ this.masterRegion = masterRegion;
+ }
+
+ /**
+ * Store the meta table name in the Master Local Region. This should be
called once during cluster
+ * initialization. The stored value is cluster-specific and should not
conflict with other
+ * clusters sharing the same HDFS.
+ * @param metaTableName the meta table name to store
+ * @throws IOException if the operation fails
+ */
+ public void store(TableName metaTableName) throws IOException {
+ LOG.info("Storing meta table name in Master Local Region: {}",
metaTableName);
+ Put put = new Put(META_TABLE_NAME_ROW);
+ put.addColumn(INFO_FAMILY, NAME_QUALIFIER,
Bytes.toBytes(metaTableName.getNameAsString()));
+ masterRegion.update(r -> r.put(put));
+ cachedMetaTableName = metaTableName;
+ LOG.info("Successfully stored meta table name: {}", metaTableName);
+ }
+
+ /**
+ * Load the meta table name from the Master Local Region.
+ * @return the meta table name for this cluster
+ * @throws IOException if the load operation fails
+ */
+ public TableName load() throws IOException {
+ if (cachedMetaTableName != null) {
+ return cachedMetaTableName;
+ }
+
+ synchronized (this) {
+ if (cachedMetaTableName != null) {
+ return cachedMetaTableName;
+ }
+ Get get = new Get(META_TABLE_NAME_ROW);
+ get.addColumn(INFO_FAMILY, NAME_QUALIFIER);
+ Result result = masterRegion.get(get);
+
+ if (!result.isEmpty()) {
+ byte[] value = result.getValue(INFO_FAMILY, NAME_QUALIFIER);
+ cachedMetaTableName = TableName.valueOf(Bytes.toString(value));
+ 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);
+ return cachedMetaTableName;
Review Comment:
The log message at line 89 is misleading. When no stored meta table name is
found, cachedMetaTableName is still null, but the log message says "No stored
meta table name found in Master Local Region: {}", cachedMetaTableName. This
will log null, which is confusing. The message should be clearer that no value
was found and null will be returned.
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/util/FSTableDescriptors.java:
##########
@@ -143,24 +143,30 @@ public static void
tryUpdateMetaTableDescriptor(Configuration conf) throws IOExc
CommonFSUtils.getRootDir(conf));
}
+ private static TableName getMetaTableNameFromConf(Configuration conf) {
+ // TODO: Support replica-specific meta table names from masterRegion
+ return TableName.META_TABLE_NAME;
Review 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.
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java:
##########
@@ -3672,6 +3672,22 @@ public List<HRegionLocation> getMetaLocations() {
return metaRegionLocationCache.getMetaRegionLocations();
}
+ /**
+ * RegionServers get the meta table name from Master via connection registry.
+ */
+ @Override
+ 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();
+ }
Review 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.
##########
hbase-common/src/main/java/org/apache/hadoop/hbase/TableName.java:
##########
@@ -288,8 +302,8 @@ private TableName(ByteBuffer namespace, ByteBuffer
qualifier) throws IllegalArgu
throw new IllegalArgumentException(OLD_ROOT_STR + " has been
deprecated.");
}
if (qualifierAsString.equals(OLD_META_STR)) {
- throw new IllegalArgumentException(
- OLD_META_STR + " no longer exists. The table has been " + "renamed to
" + META_TABLE_NAME);
+ throw new IllegalArgumentException(OLD_META_STR + " no longer exists.
The table has been "
+ + "renamed to hbase:meta or hbase:meta_suffix in conf");
Review Comment:
The error message at line 305-306 references "hbase:meta or
hbase:meta_suffix in conf" but this is confusing because the meta table name is
not directly configured. The message should be updated to reflect that the meta
table name is dynamically discovered, not configured. Consider: "The table has
been renamed to hbase:meta (or a cluster-specific meta table name)"
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java:
##########
@@ -1652,6 +1664,33 @@ public TableStateManager getTableStateManager() {
return tableStateManager;
}
+ /**
+ * Override base implementation to read from Master Local Region storage.
This allows the master
+ * to return the cluster-specific meta table name.
+ */
+ @Override
+ public TableName getMetaTableName() {
+ return cachedMetaTableName;
+ }
+
+ private TableName initMetaTableName() {
+ metaTableNameStore = new MetaTableNameStore(masterRegion);
+ try {
+ TableName metaTableName = metaTableNameStore.load();
+ // If metaTableNameStore is empty (bootstrap case), get meta table name
from super, store it,
+ // and return.
+ if (Objects.isNull(metaTableName)) {
+ metaTableName = super.getDefaultMetaTableName();
+ LOG.info("Bootstrap: storing default meta table name in master region:
{}", metaTableName);
+ metaTableNameStore.store(metaTableName);
+ }
+ return metaTableName;
+ } catch (IOException e) {
+ LOG.info("Exception loading/storing meta table name from master region");
+ throw new RuntimeException(e);
+ }
Review 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.
##########
hbase-common/src/main/java/org/apache/hadoop/hbase/TableName.java:
##########
@@ -85,9 +90,18 @@ public final class TableName implements
Comparable<TableName> {
/** One globally disallowed name */
public static final String DISALLOWED_TABLE_NAME = "zookeeper";
- /** Returns True if <code>tn</code> is the hbase:meta table name. */
+ /**
+ * Returns True if <code>tn</code> is a meta table (hbase:meta or
hbase:meta_suffix). This handles
+ * both the default meta table and read replica meta tables.
+ */
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_");
}
Review Comment:
The isMetaTableName() implementation at lines 97-105 uses
startsWith("meta_") which could incorrectly match table names like
"hbase:meta_data" or "hbase:meta_table". The implementation should ensure
there's a valid suffix delimiter or use exact string matching. The comment
mentions "read replica meta tables" but the actual format/pattern for these
replica table names is not clear. Consider either documenting the expected
pattern or using a more restrictive check (e.g., checking for specific known
suffixes like "meta_replica1", etc.).
##########
hbase-client/src/main/java/org/apache/hadoop/hbase/client/RegionInfoBuilder.java:
##########
@@ -42,6 +42,8 @@ public class RegionInfoBuilder {
*/
// 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
Review Comment:
The comment on line 46 has a typo: "conenction" should be "connection".
```suggestion
// connection
```
##########
hbase-thrift/src/main/java/org/apache/hadoop/hbase/thrift2/client/ThriftConnection.java:
##########
@@ -369,6 +369,11 @@ public void clearRegionLocationCache() {
throw new NotImplementedException("clearRegionLocationCache not supported
in ThriftTable");
}
+ @Override
+ public TableName getMetaTableName() {
+ return toAsyncConnection().getMetaTableName();
+ }
Review 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().
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/master/janitor/MetaFixer.java:
##########
@@ -203,19 +203,20 @@ private static List<RegionInfo> createMetaEntries(final
MasterServices masterSer
.flatMap(List::stream).collect(Collectors.toList());
final List<IOException> createMetaEntriesFailures =
addMetaEntriesResults.stream()
.filter(Either::hasRight).map(Either::getRight).collect(Collectors.toList());
- LOG.debug("Added {}/{} entries to hbase:meta",
createMetaEntriesSuccesses.size(),
- newRegionInfos.size());
+ LOG.debug("Added {}/{} entries to {}", createMetaEntriesSuccesses.size(),
newRegionInfos.size(),
+ TableName.META_TABLE_NAME.getNameAsString());
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"
Review Comment:
The log parameter format string has a typo - there's an extra closing brace.
The format string shows "Failed to create entries in {}} for {}/{}" but should
be "Failed to create entries in {} for {}/{}" with only one closing brace after
the first placeholder.
--
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]
