HBASE-23055 Alter hbase:meta #667
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
saintstack
commented
Sep 27, 2019
saintstack
commented
|
💔 -1 overall
This message was automatically generated. |
saintstack
force-pushed
the
HBASE-23055.master
branch
from
0f29cc4 to
316f90a
Compare
|
💔 -1 overall
This message was automatically generated. |
Make it so hbase:meta can be altered. TableState for hbase:meta
is kept in Master. State is in-memory transient so if Master
fails, hbase:meta is ENABLED again. hbase:meta schema will be
bootstrapped from the filesystem. Changes to filesystem schema
are atomic so we should be ok if Master fails mid-edit (TBD)
Undoes a bunch of guards that prevented our being able to edit
hbase:meta. At minimmum, need to add in a bunch of WARNING.
TODO: Tests, more clarity around hbase:meta table state, and undoing
references to hard-coded hbase:meta regioninfo.
M hbase-client/src/main/java/org/apache/hadoop/hbase/MetaTableAccessor.java
Throw illegal access exception if you try to use MetaTableAccessor
getting state of the hbase:meta table.
M hbase-client/src/main/java/org/apache/hadoop/hbase/client/ConnectionImplementation.java
For table state, go to master rather than go to meta direct. Going
to meta won't work for hbase;meta state. Puts load on Master.
M hbase-client/src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java
Change isTableDisabled/Enabled implementation to ask the Master instead.
This will give the Master's TableStateManager's opinion rather than
client figuring it for themselves reading meta table direct.
M hbase-client/src/main/java/org/apache/hadoop/hbase/client/RawAsyncHBaseAdmin.java
TODO: Cleanup in here. Go to master for state, not to meta.
M hbase-client/src/main/java/org/apache/hadoop/hbase/client/ZKAsyncRegistry.java
Logging cleanup.
M hbase-client/src/main/java/org/apache/hadoop/hbase/zookeeper/ZNodePaths.java
Shutdown access.
M hbase-server/src/main/java/org/apache/hadoop/hbase/TableDescriptors.java
Just cleanup.
M hbase-server/src/main/java/org/apache/hadoop/hbase/master/TableStateManager.java
Add state holder for hbase:meta.
Removed unused methods.
M hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/RegionStateStore.java
Shut down access.
M hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/DisableTableProcedure.java
Allow hbase:meta to be disabled.
M hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/EnableTableProcedure.java
Allow hbase:meta to be enabled.
saintstack
force-pushed
the
HBASE-23055.master
branch
from
316f90a to
8101068
Compare
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
Apache9
reviewed
Oct 1, 2019
Contributor
Apache9
left a comment
Apache9
left a comment
There was a problem hiding this comment.
Sorry a bit late but this has already been committed to master and branch-2? I thought this will be on a feature branch... This is really a big change, I do not think we should commit this to branch-2...
| import org.apache.hadoop.hbase.security.access.Permission; | ||
| import org.apache.hadoop.hbase.security.access.ShadedAccessControlUtil; | ||
| import org.apache.hadoop.hbase.security.access.UserPermission; | ||
|
|
| 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"); |
Contributor
There was a problem hiding this comment.
IllegalArgumentException? An Error seems over kill.
Contributor
Author
There was a problem hiding this comment.
Nah. This is helpful figuring all the places where we are asking MetaTableAccessor for hbase:meta state. It is an error if a client asks MTA for state of hbase:meta.
| * @return Future that calls Master getTableState and compares to <code>state</code> | ||
| */ | ||
| private CompletableFuture<Boolean> isTableState(TableName tableName, TableState.State state) { | ||
| return this.<Boolean> newMasterCaller(). |
Contributor
There was a problem hiding this 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.
Contributor
Author
There was a problem hiding this comment.
Let me file subtask to look at the incidence of state lookups.
I was thinking admin enable/disable are infrequent and ok if they fail because master is offline. Let me look at how often inline read/write does table state. Thanks.
| * 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; |
Contributor
There was a problem hiding this comment.
Why not store it on zk? Just like the location of meta table...
Contributor
Author
There was a problem hiding this comment.
If the Master dies during enable/disable, I want it to default to being online when new Master comes online. Lots of new conditions if hbase:meta is offline.
Contributor
Author
There was a problem hiding this comment.
hbase:meta being offline should be temporary state.
| * @param states states to check against | ||
| * @return null if succeed or table state if failed | ||
| */ | ||
| public TableState setTableStateIfInStates(TableName tableName, TableState.State newState, |
| */ | ||
| public EnableTableProcedure(MasterProcedureEnv env, TableName tableName, | ||
| ProcedurePrepareLatch syncLatch) { | ||
| ProcedurePrepareLatch syncLatch) { |
| throws ExecutionException, InterruptedException { | ||
| AsyncTable t = connection.getTable(TableName.META_TABLE_NAME); | ||
| List<HRegionLocation> rls = | ||
| t.getRegionLocator().getRegionLocations(HConstants.EMPTY_START_ROW, true).get(); |
Contributor
There was a problem hiding this comment.
Use FutureUtils.get? It will unwrap the ExecutionException.
| @@ -1,4 +1,6 @@ | |||
| /** | |||
| /* | |||
| * Copyright The Apache Software Foundation | |||
Contributor
Author
There was a problem hiding this comment.
Because it is not javadoc.
|
|
||
| /** | ||
| * Should be private | ||
| * @deprecated Since 2.3.0. Should be for internal use only. Used by testing. |
Contributor
There was a problem hiding this comment.
This class is IA.Private so we can remove this method at any time.
|
|
||
| import org.apache.hadoop.hbase.shaded.protobuf.ProtobufUtil; | ||
|
|
||
|
|
Contributor
There was a problem hiding this comment.
Nit: remove useless empty line.
Contributor
Author
There was a problem hiding this comment.
Let me address review in a sub-issue. I want to get a test run in to make sure nothing broke.
asfgit
pushed a commit
that referenced
this pull request
Oct 1, 2019
Revert because new feedback and requested survey of master usage figuring table state. This reverts commit 5217618.
asfgit
pushed a commit
that referenced
this pull request
Oct 1, 2019
Make it so hbase:meta can be altered. TableState for hbase:meta
is kept in Master. State is in-memory transient so if Master
fails, hbase:meta is ENABLED again. hbase:meta schema will be
bootstrapped from the filesystem. Changes to filesystem schema
are atomic so we should be ok if Master fails mid-edit (TBD)
Undoes a bunch of guards that prevented our being able to edit
hbase:meta. At minimmum, need to add in a bunch of WARNING.
TODO: Tests, more clarity around hbase:meta table state, and undoing
references to hard-coded hbase:meta regioninfo.
M hbase-client/src/main/java/org/apache/hadoop/hbase/MetaTableAccessor.java
Throw illegal access exception if you try to use MetaTableAccessor
getting state of the hbase:meta table.
M hbase-client/src/main/java/org/apache/hadoop/hbase/client/ConnectionImplementation.java
For table state, go to master rather than go to meta direct. Going
to meta won't work for hbase;meta state. Puts load on Master.
M hbase-client/src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java
Change isTableDisabled/Enabled implementation to ask the Master instead.
This will give the Master's TableStateManager's opinion rather than
client figuring it for themselves reading meta table direct.
M hbase-client/src/main/java/org/apache/hadoop/hbase/client/RawAsyncHBaseAdmin.java
TODO: Cleanup in here. Go to master for state, not to meta.
M hbase-client/src/main/java/org/apache/hadoop/hbase/client/ZKAsyncRegistry.java
Logging cleanup.
M hbase-client/src/main/java/org/apache/hadoop/hbase/zookeeper/ZNodePaths.java
Shutdown access.
M hbase-server/src/main/java/org/apache/hadoop/hbase/TableDescriptors.java
Just cleanup.
M hbase-server/src/main/java/org/apache/hadoop/hbase/master/TableStateManager.java
Add state holder for hbase:meta.
Removed unused methods.
M hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/RegionStateStore.java
Shut down access.
M hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/DisableTableProcedure.java
Allow hbase:meta to be disabled.
M hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/EnableTableProcedure.java
Allow hbase:meta to be enabled.
Signed-off-by: Ramkrishna <ramkrishna.s.vasudevan@intel.com>
Contributor
Author
|
Reverted from master and branch-2. Made feature branch HBASE-23055. The first commit on the feature branch includes change that addresses the @Apache9 feedback above. |
asfgit
pushed a commit
that referenced
this pull request
Oct 4, 2019
Make it so hbase:meta can be altered. TableState for hbase:meta
is kept in Master. State is in-memory transient so if Master
fails, hbase:meta is ENABLED again. hbase:meta schema will be
bootstrapped from the filesystem. Changes to filesystem schema
are atomic so we should be ok if Master fails mid-edit (TBD)
Undoes a bunch of guards that prevented our being able to edit
hbase:meta. At minimmum, need to add in a bunch of WARNING.
TODO: Tests, more clarity around hbase:meta table state, and undoing
references to hard-coded hbase:meta regioninfo.
M hbase-client/src/main/java/org/apache/hadoop/hbase/MetaTableAccessor.java
Throw illegal access exception if you try to use MetaTableAccessor
getting state of the hbase:meta table.
M hbase-client/src/main/java/org/apache/hadoop/hbase/client/ConnectionImplementation.java
For table state, go to master rather than go to meta direct. Going
to meta won't work for hbase;meta state. Puts load on Master.
M hbase-client/src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java
Change isTableDisabled/Enabled implementation to ask the Master instead.
This will give the Master's TableStateManager's opinion rather than
client figuring it for themselves reading meta table direct.
M hbase-client/src/main/java/org/apache/hadoop/hbase/client/RawAsyncHBaseAdmin.java
TODO: Cleanup in here. Go to master for state, not to meta.
M hbase-client/src/main/java/org/apache/hadoop/hbase/client/ZKAsyncRegistry.java
Logging cleanup.
M hbase-client/src/main/java/org/apache/hadoop/hbase/zookeeper/ZNodePaths.java
Shutdown access.
M hbase-server/src/main/java/org/apache/hadoop/hbase/TableDescriptors.java
Just cleanup.
M hbase-server/src/main/java/org/apache/hadoop/hbase/master/TableStateManager.java
Add state holder for hbase:meta.
Removed unused methods.
M hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/RegionStateStore.java
Shut down access.
M hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/DisableTableProcedure.java
Allow hbase:meta to be disabled.
M hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/EnableTableProcedure.java
Allow hbase:meta to be enabled.
Signed-off-by: Ramkrishna <ramkrishna.s.vasudevan@intel.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.