kgeisz commented on code in PR #7730:
URL: https://github.com/apache/hbase/pull/7730#discussion_r2790889178
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/ModifyTableProcedure.java:
##########
@@ -110,7 +110,7 @@ protected void preflightChecks(MasterProcedureEnv env,
Boolean enabled) throws H
for (byte[] family : UNDELETABLE_META_COLUMNFAMILIES) {
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));
Review Comment:
nit: IMO this could be a little more clear
```suggestion
"Invalid deletion of " + env.getMetaTableName() + " column
family: " + Bytes.toString(family));
```
##########
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:
nit:
```suggestion
// connection
```
##########
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:
nit
```suggestion
"Failed to create entries in {} for {}/{} RegionInfo descriptors.
First"
```
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/util/HBaseFsck.java:
##########
@@ -2224,7 +2231,8 @@ else if (!inMeta && !inHdfs && !isDeployed) {
return;
}
- LOG.info("Patching hbase:meta with with .regioninfo: " +
hbi.getHdfsHRI());
+ LOG.info("Patching {} with with .regioninfo: " + hbi.getHdfsHRI(),
+ connection.getMetaTableName());
Review Comment:
nit: Mixture of string concatenation and `{}` in log method
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/tool/BulkLoadHFilesTool.java:
##########
@@ -660,21 +660,21 @@ private int getRegionIndex(List<Pair<byte[], byte[]>>
startEndKeys, byte[] key)
private void checkRegionIndexValid(int idx, List<Pair<byte[], byte[]>>
startEndKeys,
TableName tableName) throws IOException {
if (idx < 0) {
- throw new IOException("The first region info for table " + tableName
- + " can't be found in hbase:meta.Please use hbck tool to fix it
first.");
+ throw new IOException("The first region info for table " + tableName + "
can't be found in "
+ + "hbase:meta. Please use hbck tool to fix it first.");
Review Comment:
Just want to confirm - Are we okay with hard-coding `hbase:meta` here?
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/util/HBaseFsck.java:
##########
@@ -2196,7 +2202,8 @@ else if (!inMeta && !inHdfs && !isDeployed) {
}
}
}
- LOG.info("Patching hbase:meta with .regioninfo: " + hbi.getHdfsHRI());
+ LOG.info("Patching {} with .regioninfo: " + hbi.getHdfsHRI(),
+ connection.getMetaTableName());
Review Comment:
nit: Mixture of string concatenation and `{}` in log method
##########
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.
Review Comment:
nit: Is this comment still applicable?
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/util/HBaseFsck.java:
##########
@@ -2019,9 +2021,11 @@ void closeRegion(HbckRegionInfo hi) throws IOException,
InterruptedException {
}
RegionInfo hri = h.getRegion();
if (hri == null) {
- LOG.warn("Unable to close region " + hi.getRegionNameAsString()
- + " because hbase:meta had invalid or missing " +
HConstants.CATALOG_FAMILY_STR + ":"
- + Bytes.toString(HConstants.REGIONINFO_QUALIFIER) + " qualifier
value.");
+ 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());
Review Comment:
nit: I see a mixture of using string concatenation as well as `{}` in the
`LOG.warn()` method. IMO is should consistently use the `{}` convention.
##########
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);
Review Comment:
nit
```suggestion
LOG.info("No stored meta table name found in Master Local Region: {}",
cachedMetaTableName);
```
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/tool/BulkLoadHFilesTool.java:
##########
@@ -660,21 +660,21 @@ private int getRegionIndex(List<Pair<byte[], byte[]>>
startEndKeys, byte[] key)
private void checkRegionIndexValid(int idx, List<Pair<byte[], byte[]>>
startEndKeys,
TableName tableName) throws IOException {
if (idx < 0) {
- throw new IOException("The first region info for table " + tableName
- + " can't be found in hbase:meta.Please use hbck tool to fix it
first.");
+ throw new IOException("The first region info for table " + tableName + "
can't be found in "
+ + "hbase:meta. Please use hbck tool to fix it first.");
} else if (
(idx == startEndKeys.size() - 1)
&& !Bytes.equals(startEndKeys.get(idx).getSecond(),
HConstants.EMPTY_BYTE_ARRAY)
) {
- throw new IOException("The last region info for table " + tableName
- + " can't be found in hbase:meta.Please use hbck tool to fix it
first.");
+ throw new IOException("The last region info for table " + tableName + "
can't be found in "
+ + "hbase:meta. Please use hbck tool to fix it first.");
Review Comment:
Same with here
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/util/HBaseFsck.java:
##########
@@ -2301,9 +2309,9 @@ else if (!inMeta && !inHdfs && !isDeployed) {
}
} else if (inMeta && inHdfs && isMultiplyDeployed) {
errors.reportError(ERROR_CODE.MULTI_DEPLOYED,
- "Region " + descriptiveName + " is listed in hbase:meta on region
server "
- + 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"
Review Comment:
nit: What does "multiply assigned" mean? Like "multi-deployed"?
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/tool/BulkLoadHFilesTool.java:
##########
@@ -660,21 +660,21 @@ private int getRegionIndex(List<Pair<byte[], byte[]>>
startEndKeys, byte[] key)
private void checkRegionIndexValid(int idx, List<Pair<byte[], byte[]>>
startEndKeys,
TableName tableName) throws IOException {
if (idx < 0) {
- throw new IOException("The first region info for table " + tableName
- + " can't be found in hbase:meta.Please use hbck tool to fix it
first.");
+ throw new IOException("The first region info for table " + tableName + "
can't be found in "
+ + "hbase:meta. Please use hbck tool to fix it first.");
} else if (
(idx == startEndKeys.size() - 1)
&& !Bytes.equals(startEndKeys.get(idx).getSecond(),
HConstants.EMPTY_BYTE_ARRAY)
) {
- throw new IOException("The last region info for table " + tableName
- + " can't be found in hbase:meta.Please use hbck tool to fix it
first.");
+ throw new IOException("The last region info for table " + tableName + "
can't be found in "
+ + "hbase:meta. Please use hbck tool to fix it first.");
} else if (
idx + 1 < startEndKeys.size() &&
!(Bytes.compareTo(startEndKeys.get(idx).getSecond(),
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."
Review Comment:
And here
--
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]
