apurtell commented on a change in pull request #3287:
URL: https://github.com/apache/hbase/pull/3287#discussion_r636405262
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/util/FSTableDescriptors.java
##########
@@ -223,6 +253,21 @@ public TableDescriptor get(TableName tableName) {
} catch (NullPointerException | IOException ioe) {
LOG.debug("Exception during readTableDecriptor. Current table name = " +
tableName, ioe);
}
+
+ // Validate whether meta table descriptor is in HBase 2.x format
+ if (TableName.isMetaTableName(tableName) && defaultMetaTableDesc != null) {
+ try {
+ validateMetaTableDescriptor(tdmt);
+ // FS have proper meta table descriptor, we don't need to validate it
again.
+ // Reset defaultMetaTableDesc
+ defaultMetaTableDesc = null;
+ } catch (TableInfoMissingException | NoSuchColumnFamilyException e) {
+ // Meta is still in old format, return the default meta table
descriptor util we have meta
+ // descriptor in HBase 2.x format
+ return defaultMetaTableDesc;
Review comment:
This seems wrong.
`createMetaTableDescriptorBuilder` should return a builder for what the
current version of the code expects.
If we need fallbacks to ride over an upgrade case, those fallbacks should be
implemented where the errors are happening.
##########
File path: hbase-common/src/main/java/org/apache/hadoop/hbase/HConstants.java
##########
@@ -1601,6 +1601,16 @@
*/
public static final int BATCH_ROWS_THRESHOLD_DEFAULT = 5000;
+ /**
+ * List of column families that cannot be deleted from the hbase:meta table.
+ * They are critical to cluster operation. This is a bit of an odd place to
+ * keep this list but then this is the tooling that does add/remove. Keeping
+ * it local!
Review comment:
This comment is wrong, right? We don't have any tooling in HConstants.
This should go where table descriptors are defined and managed.
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/util/FSTableDescriptors.java
##########
@@ -94,6 +95,10 @@
// TODO.
private final Map<TableName, TableDescriptor> cache = new
ConcurrentHashMap<>();
+ // Default meta table descriptor, will be used by RegionServer during
rolling upgrade until
+ // HMaster write latest 2.x meta table descriptor
+ private TableDescriptor defaultMetaTableDesc = null;
Review comment:
What happens when using this "default descriptor" the regionserver
attempts to update meta? Can that happen? We are assuming the master will
rewrite soon, but is that valid? Regionservers don't run masters, they rely on
an operator to do that. Who knows what the operator is doing.
Would it be better to address specific fallbacks where something is missing
or not expected?
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/util/FSTableDescriptors.java
##########
@@ -123,8 +136,10 @@ public static TableDescriptor
tryUpdateAndGetMetaTableDescriptor(Configuration c
FileSystem fs, Path rootdir) throws IOException {
// see if we already have meta descriptor on fs. Write one if not.
try {
- return getTableDescriptorFromFs(fs, rootdir, TableName.META_TABLE_NAME);
- } catch (TableInfoMissingException e) {
+ TableDescriptor td = getTableDescriptorFromFs(fs, rootdir,
TableName.META_TABLE_NAME);
+ validateMetaTableDescriptor(td);
+ return td;
+ } catch (TableInfoMissingException | NoSuchColumnFamilyException e) {
Review comment:
This makes sense. The table might be missing altogether (what current
code handles), or might be missing a column family due to legacy (what this
change handles)
--
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.
For queries about this service, please contact Infrastructure at:
[email protected]
