[GitHub] [hbase] Apache9 commented on a change in pull request #667: HBASE-23055 Alter hbase:meta
Apache9 commented on a change in pull request #667: HBASE-23055 Alter hbase:meta
URL: https://github.com/apache/hbase/pull/667#discussion_r330042034
##
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/MetaTableAccessor.java
##
@@ -1077,7 +1084,7 @@ public static RegionInfo getRegionInfo(final Result r,
byte [] qualifier) {
public static TableState getTableState(Connection conn, TableName tableName)
throws IOException {
if (tableName.equals(TableName.META_TABLE_NAME)) {
- return new TableState(tableName, TableState.State.ENABLED);
+ throw new IllegalAccessError("Go to the Master to find hbase:meta table
state, not here");
Review comment:
IllegalArgumentException? An Error seems over kill.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase] Apache9 commented on a change in pull request #667: HBASE-23055 Alter hbase:meta
Apache9 commented on a change in pull request #667: HBASE-23055 Alter hbase:meta URL: https://github.com/apache/hbase/pull/667#discussion_r330041404 ## File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/RawAsyncHBaseAdmin.java ## @@ -90,6 +90,7 @@ import org.apache.hadoop.hbase.security.access.Permission; import org.apache.hadoop.hbase.security.access.ShadedAccessControlUtil; import org.apache.hadoop.hbase.security.access.UserPermission; + Review comment: Why an empty line? 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [hbase] Apache9 commented on a change in pull request #667: HBASE-23055 Alter hbase:meta
Apache9 commented on a change in pull request #667: HBASE-23055 Alter hbase:meta
URL: https://github.com/apache/hbase/pull/667#discussion_r330096522
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/EnableTableProcedure.java
##
@@ -175,40 +166,32 @@ protected Flow executeFromState(final MasterProcedureEnv
env, final EnableTableS
default:
throw new UnsupportedOperationException("unhandled state=" + state);
}
-} catch (IOException e) {
+} catch (IOException | ExecutionException e) {
if (isRollbackSupported(state)) {
-setFailure("master-enable-table", e);
+setFailure("master-enable-table", getCause(e));
} else {
-LOG.warn(
- "Retriable error trying to enable table=" + tableName + " (in
state=" + state + ")", e);
+LOG.warn("Retryable error enabling {}, state={}", tableName, state,
getCause(e));
}
}
return Flow.HAS_MORE_STATE;
}
- private int getNumberOfReplicasFromMeta(Connection connection, int
regionReplicaCount,
- List regionsOfTable) throws IOException {
-Result r = getRegionFromMeta(connection, regionsOfTable);
-int replicasFound = 0;
-for (int i = 1; i < regionReplicaCount; i++) {
- // Since we have already added the entries to the META we will be
getting only that here
- List columnCells =
- r.getColumnCells(HConstants.CATALOG_FAMILY,
MetaTableAccessor.getServerColumn(i));
- if (!columnCells.isEmpty()) {
-replicasFound++;
- }
-}
-return replicasFound;
+ /**
+ * @return If ExecutionException, pull out the cause.
+ */
+ private Throwable getCause(Exception e) {
+return e instanceof ExecutionException?
((ExecutionException)e).getCause(): e;
}
- private Result getRegionFromMeta(Connection connection, List
regionsOfTable)
- throws IOException {
-byte[] metaKeyForRegion =
MetaTableAccessor.getMetaKeyForRegion(regionsOfTable.get(0));
-Get get = new Get(metaKeyForRegion);
-get.addFamily(HConstants.CATALOG_FAMILY);
-Table metaTable = MetaTableAccessor.getMetaHTable(connection);
-Result r = metaTable.get(get);
-return r;
+ /**
+ * @return If hbase;meta table, it goes to the registry implementation which
is what we want.
+ */
+ private int getReplicaCount(AsyncConnection connection, TableName tableName)
+ throws ExecutionException, InterruptedException {
+AsyncTable t = connection.getTable(TableName.META_TABLE_NAME);
+List rls =
+t.getRegionLocator().getRegionLocations(HConstants.EMPTY_START_ROW,
true).get();
Review comment:
Use FutureUtils.get? It will unwrap the ExecutionException.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase] Apache9 commented on a change in pull request #667: HBASE-23055 Alter hbase:meta
Apache9 commented on a change in pull request #667: HBASE-23055 Alter hbase:meta URL: https://github.com/apache/hbase/pull/667#discussion_r330097219 ## File path: hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/RejectReplicationRequestStateChecker.java ## @@ -1,4 +1,6 @@ -/** +/* + * Copyright The Apache Software Foundation Review comment: Why this line? 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [hbase] Apache9 commented on a change in pull request #667: HBASE-23055 Alter hbase:meta
Apache9 commented on a change in pull request #667: HBASE-23055 Alter hbase:meta URL: https://github.com/apache/hbase/pull/667#discussion_r330094572 ## File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/TableStateManager.java ## @@ -87,61 +98,6 @@ public void setTableState(TableName tableName, TableState.State newState) throws } } - /** - * Set table state to provided but only if table in specified states Caller should lock table on - * write. - * @param tableName table to change state for - * @param newState new state - * @param states states to check against - * @return null if succeed or table state if failed - */ - public TableState setTableStateIfInStates(TableName tableName, TableState.State newState, Review comment: Not used? 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [hbase] Apache9 commented on a change in pull request #667: HBASE-23055 Alter hbase:meta
Apache9 commented on a change in pull request #667: HBASE-23055 Alter hbase:meta
URL: https://github.com/apache/hbase/pull/667#discussion_r330043419
##
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/RawAsyncHBaseAdmin.java
##
@@ -666,42 +669,25 @@ public void run(PRESP resp) {
@Override
public CompletableFuture isTableEnabled(TableName tableName) {
-if (TableName.isMetaTableName(tableName)) {
- return CompletableFuture.completedFuture(true);
-}
-CompletableFuture future = new CompletableFuture<>();
-addListener(AsyncMetaTableAccessor.getTableState(metaTable, tableName),
(state, error) -> {
- if (error != null) {
-future.completeExceptionally(error);
-return;
- }
- if (state.isPresent()) {
-future.complete(state.get().inStates(TableState.State.ENABLED));
- } else {
-future.completeExceptionally(new TableNotFoundException(tableName));
- }
-});
-return future;
+return isTableState(tableName, TableState.State.ENABLED);
}
@Override
public CompletableFuture isTableDisabled(TableName tableName) {
-if (TableName.isMetaTableName(tableName)) {
- return CompletableFuture.completedFuture(false);
-}
-CompletableFuture future = new CompletableFuture<>();
-addListener(AsyncMetaTableAccessor.getTableState(metaTable, tableName),
(state, error) -> {
- if (error != null) {
-future.completeExceptionally(error);
-return;
- }
- if (state.isPresent()) {
-future.complete(state.get().inStates(TableState.State.DISABLED));
- } else {
-future.completeExceptionally(new TableNotFoundException(tableName));
- }
-});
-return future;
+return isTableState(tableName, TableState.State.DISABLED);
+ }
+
+ /**
+ * @return Future that calls Master getTableState and compares to
state
+ */
+ private CompletableFuture isTableState(TableName tableName,
TableState.State state) {
+return this. newMasterCaller().
Review comment:
Is this really a good degisn? In the normal read/write path, sometimes we
need to test whether a table is disabled. After this change, the normal
read/write path will also rely on master, while in the past we only need to
access meta table.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase] Apache9 commented on a change in pull request #667: HBASE-23055 Alter hbase:meta
Apache9 commented on a change in pull request #667: HBASE-23055 Alter hbase:meta
URL: https://github.com/apache/hbase/pull/667#discussion_r330095413
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/EnableTableProcedure.java
##
@@ -66,11 +69,12 @@ public EnableTableProcedure(MasterProcedureEnv env,
TableName tableName) {
/**
* Constructor
- * @param env MasterProcedureEnv
+ *
+ * @param env MasterProcedureEnv
* @param tableName the table to operate on
*/
public EnableTableProcedure(MasterProcedureEnv env, TableName tableName,
- ProcedurePrepareLatch syncLatch) {
+ ProcedurePrepareLatch syncLatch) {
Review comment:
Format issue?
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase] Apache9 commented on a change in pull request #667: HBASE-23055 Alter hbase:meta
Apache9 commented on a change in pull request #667: HBASE-23055 Alter hbase:meta
URL: https://github.com/apache/hbase/pull/667#discussion_r330097961
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/util/FSTableDescriptors.java
##
@@ -123,30 +119,22 @@ public FSTableDescriptors(final Configuration conf,
final FileSystem fs, final P
* operations; i.e. on remove, we do not do delete in fs.
*/
public FSTableDescriptors(final Configuration conf, final FileSystem fs,
-final Path rootdir, final boolean fsreadonly,
final boolean usecache) throws IOException {
-this(conf, fs, rootdir, fsreadonly, usecache, null);
- }
-
- /**
- * @param fsreadonly True if we are read-only when it comes to filesystem
- * operations; i.e. on remove, we do not do delete in fs.
- * @param metaObserver Used by HMaster. It need to modify the
META_REPLICAS_NUM for meta table descriptor.
- * see HMaster#finishActiveMasterInitialization
- * TODO: This is a workaround. Should remove this ugly
code...
- */
- public FSTableDescriptors(final Configuration conf, final FileSystem fs,
-final Path rootdir, final boolean fsreadonly,
final boolean usecache,
-Function metaObserver) throws IOException {
+ final Path rootdir, final boolean fsreadonly, final boolean usecache)
throws IOException {
this.fs = fs;
this.rootdir = rootdir;
this.fsreadonly = fsreadonly;
this.usecache = usecache;
-this.metaTableDescriptor = metaObserver == null ?
createMetaTableDescriptor(conf)
- : metaObserver.apply(createMetaTableDescriptorBuilder(conf)).build();
+this.configuration = conf;
}
+ /**
+ * Should be private
+ * @deprecated Since 2.3.0. Should be for internal use only. Used by testing.
Review comment:
This class is IA.Private so we can remove this method at any time.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase] Apache9 commented on a change in pull request #667: HBASE-23055 Alter hbase:meta
Apache9 commented on a change in pull request #667: HBASE-23055 Alter hbase:meta
URL: https://github.com/apache/hbase/pull/667#discussion_r330094278
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/master/TableStateManager.java
##
@@ -53,8 +52,20 @@
// TODO: Make this a guava Service
@InterfaceAudience.Private
public class TableStateManager {
-
private static final Logger LOG =
LoggerFactory.getLogger(TableStateManager.class);
+
+ /**
+ * All table state is kept in hbase:meta except that of hbase:meta itself.
+ * hbase:meta state is kept here locally in this in-memory variable. State
+ * for hbase:meta is not persistent. If this process dies, the hbase:meta
+ * state reverts to enabled. State is used so we can edit hbase:meta as we
+ * would any other table by disabling, altering, and then re-enabling. If
this
+ * process dies in the midst of an edit, the table reverts to enabled. Schema
+ * is read from the filesystem. It is changed atomically so if we die midway
+ * through an edit we should be good.
+ */
+ private TableState.State metaTableState = TableState.State.ENABLED;
Review comment:
Why not store it on zk? Just like the location of meta table...
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase] Apache9 commented on a change in pull request #667: HBASE-23055 Alter hbase:meta
Apache9 commented on a change in pull request #667: HBASE-23055 Alter hbase:meta
URL: https://github.com/apache/hbase/pull/667#discussion_r330095237
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/DisableTableProcedure.java
##
@@ -242,10 +241,7 @@ public TableOperationType getTableOperationType() {
*/
private boolean prepareDisable(final MasterProcedureEnv env) throws
IOException {
boolean canTableBeDisabled = true;
-if (tableName.equals(TableName.META_TABLE_NAME)) {
- setFailure("master-disable-table", new ConstraintException("Cannot
disable catalog table"));
- canTableBeDisabled = false;
-} else if
(!MetaTableAccessor.tableExists(env.getMasterServices().getConnection(),
tableName)) {
+if
(!MetaTableAccessor.tableExists(env.getMasterServices().getConnection(),
tableName)) {
Review comment:
MetaTableAccessor can be used to test whether meta table exists?
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase] Apache9 commented on a change in pull request #667: HBASE-23055 Alter hbase:meta
Apache9 commented on a change in pull request #667: HBASE-23055 Alter hbase:meta URL: https://github.com/apache/hbase/pull/667#discussion_r330099157 ## File path: hbase-server/src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java ## @@ -157,6 +156,7 @@ import org.apache.hadoop.hbase.shaded.protobuf.ProtobufUtil; + Review comment: Nit: remove useless empty line. 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: us...@infra.apache.org With regards, Apache Git Services
