[
https://issues.apache.org/jira/browse/HBASE-23676?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17013220#comment-17013220
]
Michael Stack commented on HBASE-23676:
---------------------------------------
{code}
Duo Zhang <[email protected]>
5:33 AM (8 hours ago)
to State, apache/hbase, Michael
@Apache9 commented on this pull request.
In
hbase-client/src/main/java/org/apache/hadoop/hbase/client/RawAsyncHBaseAdmin.java:
> @@ -666,42 +667,55 @@ public void run(PRESP resp) {
new DisableTableProcedureBiConsumer(tableName));
}
+ /**
+ * Utility for completing passed TableState {@link CompletableFuture}
<code>future</code>
+ * using passed parameters.
+ */
+ private static CompletableFuture<Boolean> completeCheckTableState(
What is the return value used for?
In
hbase-client/src/main/java/org/apache/hadoop/hbase/client/ZKAsyncRegistry.java:
> @@ -58,8 +62,19 @@
private final ZNodePaths znodePaths;
+ /**
+ * A znode maintained by MirroringTableStateManager.
+ * MirroringTableStateManager is deprecated to be removed in hbase3. It can
also be disabled.
+ * Make sure it is enabled if you want to alter hbase:meta table in hbase2.
In hbase3,
+ * TBD how metatable state will be hosted; likely on active hbase master.
I think the state will either be on zk or on hdfs? HMaster itself is state
less...
In general, in the current architecture, I think it the state should be placed
on zk, of course you could cache it in master and let client go to master to
ask for the state.
In
hbase-client/src/main/java/org/apache/hadoop/hbase/client/ZKAsyncRegistry.java:
> @@ -58,8 +62,19 @@
private final ZNodePaths znodePaths;
+ /**
+ * A znode maintained by MirroringTableStateManager.
+ * MirroringTableStateManager is deprecated to be removed in hbase3. It can
also be disabled.
+ * Make sure it is enabled if you want to alter hbase:meta table in hbase2.
In hbase3,
+ * TBD how metatable state will be hosted; likely on active hbase master.
So I do not think we should name this as mirrored, it is the original data. The
state in master's memory, is a cache, actually.
In
hbase-client/src/main/java/org/apache/hadoop/hbase/client/ZKAsyncRegistry.java:
> @@ -229,6 +244,43 @@ private void
> getMetaRegionLocation(CompletableFuture<RegionLocations> future,
});
}
+ @Override
+ public CompletableFuture<TableState> getMetaTableState() {
+ return getAndConvert(this.znodeMirroredMetaTableState,
ZKAsyncRegistry::getTableState).
+ thenApply(state -> {
+ return state == null ||
state.equals(ENABLED_META_TABLE_STATE.getState())?
+ ENABLED_META_TABLE_STATE: new TableState(TableName.META_TABLE_NAME,
state);
+ }).exceptionally(e -> {
Currently in HBase, usually we will create a new CompletableFuture and use
FutureUtils.addListener to complete it. The code in exceptionally are a bit
tricky, where we throw a CompletionException...
In
hbase-server/src/main/java/org/apache/hadoop/hbase/master/TableStateManager.java:
> private static final Logger LOG =
> LoggerFactory.getLogger(TableStateManager.class);
+
+ /**
+ * All table state is kept in hbase:meta except that of hbase:meta itself.
In general I do not think this is a good design. We should persist it
somewhere, and just cache it in master's memory. And we do not need to disable
a table when altering any more? And the solution here just assume that we only
have one meta region? So the altering operation can be atomic? What if we have
multiple meta regions and we crash in the middle? I think this patch also aims
to implement splittable meta in the future? No?
In
hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/RejectReplicationRequestStateChecker.java:
> @@ -1,4 +1,6 @@
-/**
+/*
+ * Copyright The Apache Software Foundation
What's this?
In hbase-server/src/test/java/org/apache/hadoop/hbase/TestHBaseMetaEdit.java:
> + Admin admin = UTIL.getAdmin();
+ admin.tableExists(TableName.META_TABLE_NAME);
+ admin.disableTable(TableName.META_TABLE_NAME);
+ assertTrue(admin.isTableDisabled(TableName.META_TABLE_NAME));
+ TableDescriptor descriptor =
admin.getDescriptor(TableName.META_TABLE_NAME);
+ ColumnFamilyDescriptor cfd =
descriptor.getColumnFamily(HConstants.CATALOG_FAMILY);
+ byte [] extraColumnFamilyName = Bytes.toBytes("xtra");
+ ColumnFamilyDescriptor newCfd =
+
ColumnFamilyDescriptorBuilder.newBuilder(extraColumnFamilyName).build();
+ int oldVersions = cfd.getMaxVersions();
+ // Add '1' to current versions count.
+ cfd =
ColumnFamilyDescriptorBuilder.newBuilder(cfd).setMaxVersions(oldVersions + 1).
+ setConfiguration(ColumnFamilyDescriptorBuilder.DATA_BLOCK_ENCODING,
+ DataBlockEncoding.ROW_INDEX_V1.toString()).build();
+ admin.modifyColumnFamily(TableName.META_TABLE_NAME, cfd);
+ admin.addColumnFamily(TableName.META_TABLE_NAME, newCfd);
Now we allow users to add/remove families of meta table from client side? A bit
dangerous. I think we should only allow a sub set of alter operations for meta
table and system tables from client side? At least, we should not let them add
or remove a column family, it will introduce very critical problems.
In hbase-server/src/main/java/org/apache/hadoop/hbase/TableDescriptors.java:
> */
- void add(final TableDescriptor htd)
- throws IOException;
+ void add(final TableDescriptor htd) throws IOException;
This method is a bit odd. We do not call it in CreateTableProcedure, but only
in ModifyTableProcedure. Why not just call it update?
In
hbase-server/src/main/java/org/apache/hadoop/hbase/util/FSTableDescriptors.java:
> }
+ /**
+ *
+ * Should be private
+ * @deprecated Since 2.3.0. Should be for internal use only. Used by testing.
I do not think this should be deprecated? The class is IA.Private so it is OK
to put internal methods here.
{code}
> Address feedback on HBASE-23055 Alter hbase:meta.
> -------------------------------------------------
>
> Key: HBASE-23676
> URL: https://issues.apache.org/jira/browse/HBASE-23676
> Project: HBase
> Issue Type: Bug
> Affects Versions: 2.3.0
> Reporter: Michael Stack
> Assignee: Michael Stack
> Priority: Major
> Fix For: 2.3.0
>
>
> Good feedback on HBASE-23055 came in after merge from [~zhangduo]. Opening
> this issue to address it.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
