Uh oh!
There was an error while loading. Please reload this page.
HBASE-22695 Store the rsgroup of a table in table configuration - #426
Merged
Conversation
Apache9
commented
Jul 29, 2019
ContributorAuthor
Still some bugs in the balancer and can not pass TestRSGroupsAdmin2. Will update the patch soon. Create a PR first so others can see the approach on how to store rsgroup of a table in table configuration. |
Apache-HBase
commented
Jul 29, 2019
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 29, 2019
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 30, 2019
💔 -1 overall
This message was automatically generated. |
infraio
reviewed
Jul 30, 2019
| } | ||
| @Override | ||
| public RSGroupInfo getRSGroupInfoOfTable(TableName tableName) throws IOException { |
ContributorAuthor
There was a problem hiding this comment.
I think we will reference this method in hbase shell so do not want to change it in this patch...
Let's do it in another issue. And in general, if we do not want to completely remove the shell command, we'd better keep this method for a least a whole major version?
Contributor
There was a problem hiding this comment.
We can change the impl of "get_table_rsgroup" cmd? Then there will no internal usage for this.
infraio
reviewed
Jul 30, 2019
Uh oh!
There was an error while loading. Please reload this page.
infraio
reviewed
Jul 30, 2019
| if (!desc.getTableName().isSystemTable() && !rsgroupHasServersOnline(desc)) { | ||
| throw new HBaseIOException("No online servers in the rsgroup, which table " + | ||
| desc.getTableName().getNameAsString() + " belongs to"); | ||
| throw new HBaseIOException("No online servers in the rsgroup for " + desc); |
Contributor
There was a problem hiding this comment.
Why skip check for system table?
ContributorAuthor
There was a problem hiding this comment.
Just keep the old behavior. And in general, I do not think we need to check for online servers here. Region servers can die at any time and cause the rs group to have no servers in group, so users may get random behaviors and confused...
Apache-HBase
commented
Jul 30, 2019
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 31, 2019
💔 -1 overall
This message was automatically generated. |
| * the configuration of a table so this will be removed. | ||
| */ | ||
| @Deprecated | ||
| public boolean containsTable(TableName table) { |
Contributor
There was a problem hiding this comment.
This is a key method when checking if regions should be moved. Must remove it and implement a new method in moveServerRegionsFromGroup()?
ContributorAuthor
There was a problem hiding this comment.
Yes, I reimplemented the related methods. Please see RSGroupAdminServer.moveRegionsBetweenGroups.
Apache-HBase
commented
Aug 1, 2019
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 1, 2019
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 1, 2019
💔 -1 overall
This message was automatically generated. |
Apache9
commented
Aug 2, 2019
ContributorAuthor
Apache9force-pushed
the
HBASE-22695
branch
2 times, most recently
from
August 2, 2019 01:37
8933d66 to
69850cbComparesunhelly
reviewed
Aug 2, 2019
| NamespaceDescriptor nd = clusterSchema.getNamespace(tableName.getNamespaceAsString()); | ||
| String groupNameOfNs = nd.getConfigurationValue(RSGroupInfo.NAMESPACE_DESC_PROP_GROUP); | ||
| if (groupNameOfNs == null) { | ||
| return Optional.empty(); |
Contributor
There was a problem hiding this comment.
Why not return RSGroupInfo.DEFAULT_GROUP instead of empty?
ContributorAuthor
There was a problem hiding this comment.
Empty here can tell the upper layer that there is no rs group setting for this table. If we return default rs group here, the upper layer can not know if there is no rs group setting, or user set the rs group to default explicitly.
Contributor
There was a problem hiding this comment.
I thought tables without settings of rs group are in DEFAULT group. And for your concerns, I think the method name is confusing, no matter whether a table is set to default, its group info is default, but table description of rs group can be empty. Maybe we can distinguish by table description.
It doesn't matter, it's a very little problem.
ContributorAuthor
There was a problem hiding this comment.
For users, no config means in default rs group, no problem. But this is an internal method, so I think exposing more states is better.
Apache-HBase
commented
Aug 2, 2019
💔 -1 overall
This message was automatically generated. |
infraio
reviewed
Aug 2, 2019
| throw new ConstraintException("RSGroup " + name + " has " + serverCount + | ||
| " servers; you must remove these servers from the RSGroup before" + | ||
| "the RSGroup can be removed."); | ||
| " servers; you must remove these servers from the RSGroup before" + |
Contributor
There was a problem hiding this comment.
Need a space after "RSGroup before"
infraio
reviewed
Aug 2, 2019
| // check the set of servers | ||
| checkForDeadOrOnlineServers(servers); | ||
| rsGroupInfoManager.removeServers(servers); | ||
| LOG.info("Remove decommissioned servers {} from RSGroup done", servers); |
ContributorAuthor
There was a problem hiding this comment.
I haven't changed the logic here, just formatting. I guess the removeServers method is to completely remove servers from any rs groups, that;s why they use the word 'decommissioned'.
Apache-HBase
commented
Aug 2, 2019
💔 -1 overall
This message was automatically generated. |
infraio
reviewed
Aug 2, 2019
| if (optGroup.isPresent()) { | ||
| builder.setRSGroupInfo(ProtobufUtil.toProtoGroupInfo(fillTables(optGroup.get()))); | ||
| } else { | ||
| if (master.getTableStateManager().isTablePresent(tableName)) { |
Contributor
There was a problem hiding this comment.
Don't check table present for above case?
ContributorAuthor
There was a problem hiding this comment.
Only if the table is present, we can have a rs group config for the table.
asfgit pushed a commit
that referenced
this pull request
Sep 11, 2019
Signed-off-by: Guanghao Zhang <zghao@apache.org>
asfgit pushed a commit
that referenced
this pull request
Sep 11, 2019
Signed-off-by: Guanghao Zhang <zghao@apache.org>
asfgit pushed a commit
that referenced
this pull request
Sep 16, 2019
Signed-off-by: Guanghao Zhang <zghao@apache.org>
asfgit pushed a commit
that referenced
this pull request
Sep 17, 2019
Signed-off-by: Guanghao Zhang <zghao@apache.org>
asfgit pushed a commit
that referenced
this pull request
Sep 18, 2019
Signed-off-by: Guanghao Zhang <zghao@apache.org>
asfgit pushed a commit
that referenced
this pull request
Sep 20, 2019
Signed-off-by: Guanghao Zhang <zghao@apache.org>
asfgit pushed a commit
that referenced
this pull request
Sep 24, 2019
Signed-off-by: Guanghao Zhang <zghao@apache.org>
asfgit pushed a commit
that referenced
this pull request
Oct 5, 2019
Signed-off-by: Guanghao Zhang <zghao@apache.org>
asfgit pushed a commit
that referenced
this pull request
Oct 28, 2019
Signed-off-by: Guanghao Zhang <zghao@apache.org>
asfgit pushed a commit
that referenced
this pull request
Nov 5, 2019
Signed-off-by: Guanghao Zhang <zghao@apache.org>
asfgit pushed a commit
that referenced
this pull request
Nov 10, 2019
Signed-off-by: Guanghao Zhang <zghao@apache.org>
asfgit pushed a commit
that referenced
this pull request
Nov 13, 2019
Signed-off-by: Guanghao Zhang <zghao@apache.org>
asfgit pushed a commit
that referenced
this pull request
Dec 24, 2019
Signed-off-by: Guanghao Zhang <zghao@apache.org>
asfgit pushed a commit
that referenced
this pull request
Feb 2, 2020
Signed-off-by: Guanghao Zhang <zghao@apache.org>
asfgit pushed a commit
that referenced
this pull request
Feb 3, 2020
Signed-off-by: Guanghao Zhang <zghao@apache.org>
asfgit pushed a commit
that referenced
this pull request
Feb 4, 2020
Signed-off-by: Guanghao Zhang <zghao@apache.org>
asfgit pushed a commit
that referenced
this pull request
Feb 6, 2020
Signed-off-by: Guanghao Zhang <zghao@apache.org>
asfgit pushed a commit
that referenced
this pull request
Feb 7, 2020
Signed-off-by: Guanghao Zhang <zghao@apache.org>
asfgit pushed a commit
that referenced
this pull request
Feb 8, 2020
Signed-off-by: Guanghao Zhang <zghao@apache.org>
asfgit pushed a commit
that referenced
this pull request
Feb 9, 2020
Signed-off-by: Guanghao Zhang <zghao@apache.org>
asfgit pushed a commit
that referenced
this pull request
Feb 12, 2020
Signed-off-by: Guanghao Zhang <zghao@apache.org>
asfgit pushed a commit
that referenced
this pull request
Feb 20, 2020
Signed-off-by: Guanghao Zhang <zghao@apache.org>
asfgit pushed a commit
that referenced
this pull request
Feb 22, 2020
Signed-off-by: Guanghao Zhang <zghao@apache.org>
asfgit pushed a commit
that referenced
this pull request
Feb 25, 2020
Signed-off-by: Guanghao Zhang <zghao@apache.org>
asfgit pushed a commit
that referenced
this pull request
Feb 26, 2020
Signed-off-by: Guanghao Zhang <zghao@apache.org>
asfgit pushed a commit
that referenced
this pull request
Feb 29, 2020
Signed-off-by: Guanghao Zhang <zghao@apache.org>
Apache9 added a commit
that referenced
this pull request
Mar 2, 2020
Signed-off-by: Guanghao Zhang <zghao@apache.org>
thangTang pushed a commit
to thangTang/hbase
that referenced
this pull request
Apr 16, 2020
…he#426) Signed-off-by: Guanghao Zhang <zghao@apache.org>
thangTang pushed a commit
to thangTang/hbase
that referenced
this pull request
Apr 16, 2020
…he#426) Signed-off-by: Guanghao Zhang <zghao@apache.org>
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
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
4 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.
No description provided.