Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HBASE-24112 [RSGroup] Support renaming rsgroup #1435

Merged
merged 5 commits into from
Apr 13, 2020
Merged

HBASE-24112 [RSGroup] Support renaming rsgroup #1435

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9c473f0
HBASE-24112 [RSGroup] Support renaming rsgroup
Reidddddd Apr 4, 2020
6105914
Address review comments
Reidddddd Apr 6, 2020
d4163d6
Avoid reopen regions, since there's no location changes
Reidddddd Apr 6, 2020
65a7b19
Rolled back to previous version which is the right approach so far
Reidddddd Apr 7, 2020
1f2ade5
Use lambda expression and add more context in TestRSGroupsAdmin1 to e…
Reidddddd Apr 8, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions hbase-client/src/main/java/org/apache/hadoop/hbase/client/Admin.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2396,4 +2396,13 @@ Pair<List<String>, List<TableName>> getConfiguredNamespacesAndTablesInRSGroup(St
* @throws IOException if a remote or network exception occurs
*/
boolean balanceRSGroup(String groupName) throws IOException;

/**
* Rename rsgroup
* @param oldName old rsgroup name
* @param newName new rsgroup name
* @throws IOException if a remote or network exception occurs
*/
void renameRSGroup(String oldName, String newName) throws IOException;

}
5 changes: 5 additions & 0 deletions hbase-client/src/main/java/org/apache/hadoop/hbase/client/AdminOverAsyncAdmin.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1036,4 +1036,9 @@ public RSGroupInfo getRSGroup(TableName tableName) throws IOException {
public void setRSGroup(Set<TableName> tables, String groupName) throws IOException {
get(admin.setRSGroup(tables, groupName));
}

@Override
public void renameRSGroup(String oldName, String newName) throws IOException {
get(admin.renameRSGroup(oldName, newName));
}
}
8 changes: 8 additions & 0 deletions hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncAdmin.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1631,4 +1631,12 @@ CompletableFuture<List<OnlineLogRecord>> getSlowLogResponses(final Set<ServerNam
* @throws IOException if a remote or network exception occurs
*/
CompletableFuture<Boolean> balanceRSGroup(String groupName);

/**
* Rename rsgroup
* @param oldName old rsgroup name
* @param newName new rsgroup name
* @throws IOException if a remote or network exception occurs
*/
CompletableFuture<Void> renameRSGroup(String oldName, String newName);
}
5 changes: 5 additions & 0 deletions hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncHBaseAdmin.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -913,4 +913,9 @@ public CompletableFuture<RSGroupInfo> getRSGroup(TableName tableName) {
public CompletableFuture<Void> setRSGroup(Set<TableName> tables, String groupName) {
return wrap(rawAdmin.setRSGroup(tables, groupName));
}

@Override
public CompletableFuture<Void> renameRSGroup(String oldName, String newName) {
return wrap(rawAdmin.renameRSGroup(oldName, newName));
}
}
19 changes: 19 additions & 0 deletions hbase-client/src/main/java/org/apache/hadoop/hbase/client/RawAsyncHBaseAdmin.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -309,6 +309,8 @@
import org.apache.hadoop.hbase.shaded.protobuf.generated.RSGroupAdminProtos.RemoveRSGroupResponse;
import org.apache.hadoop.hbase.shaded.protobuf.generated.RSGroupAdminProtos.RemoveServersRequest;
import org.apache.hadoop.hbase.shaded.protobuf.generated.RSGroupAdminProtos.RemoveServersResponse;
import org.apache.hadoop.hbase.shaded.protobuf.generated.RSGroupAdminProtos.RenameRSGroupRequest;
import org.apache.hadoop.hbase.shaded.protobuf.generated.RSGroupAdminProtos.RenameRSGroupResponse;
import org.apache.hadoop.hbase.shaded.protobuf.generated.ReplicationProtos.AddReplicationPeerRequest;
import org.apache.hadoop.hbase.shaded.protobuf.generated.ReplicationProtos.AddReplicationPeerResponse;
import org.apache.hadoop.hbase.shaded.protobuf.generated.ReplicationProtos.DisableReplicationPeerRequest;
Expand Down Expand Up @@ -4146,4 +4148,21 @@ public CompletableFuture<RSGroupInfo> getRSGroup(String groupName) {
resp -> resp.hasRSGroupInfo() ? ProtobufUtil.toGroupInfo(resp.getRSGroupInfo()) : null)))
.call();
}

@Override
public CompletableFuture<Void> renameRSGroup(String oldName, String newName) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you like to provide any validation for both strings? e.g. non-empty

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is ok to leave it as it is, server side will throw ConstraintException if both are empty. Other methods like addRSGroup neither checks non-empty.

return this.<Void> newMasterCaller()
.action(
(
(controller, stub) -> this.<RenameRSGroupRequest, RenameRSGroupResponse, Void> call(
controller,
stub,
RenameRSGroupRequest.newBuilder().setOldRsgroupName(oldName).setNewRsgroupName(newName)
.build(),
(s, c, req, done) -> s.renameRSGroup(c, req, done),
resp -> null
)
)
).call();
}
}
3 changes: 3 additions & 0 deletions hbase-common/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupInfo.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,9 @@ public class RSGroupInfo {
// Keep servers in a sorted set so has an expected ordering when displayed.
private final SortedSet<Address> servers;
// Keep tables sorted too.

// TODO: Don't understand why all these should be deprecated. we have table -> rsgroup mapping.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because we do not store the table informations in RSGroupInfo anymore, if you use the new API to get RSGroupInfo, the tables will always be empty.

// Isn't it reasonable to have rsgroup -> tables mapping as well. Any contradictory?
/**
* @deprecated Since 3.0.0, will be removed in 4.0.0. The rsgroup information will be stored in
* the configuration of a table so this will be removed.
Expand Down
3 changes: 3 additions & 0 deletions hbase-protocol-shaded/src/main/protobuf/server/master/Master.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1119,6 +1119,9 @@ service MasterService {

rpc GetConfiguredNamespacesAndTablesInRSGroup(GetConfiguredNamespacesAndTablesInRSGroupRequest)
returns (GetConfiguredNamespacesAndTablesInRSGroupResponse);

rpc RenameRSGroup(RenameRSGroupRequest)
returns (RenameRSGroupResponse);
}

// HBCK Service definitions.
Expand Down
11 changes: 11 additions & 0 deletions hbase-protocol-shaded/src/main/protobuf/server/rsgroup/RSGroupAdmin.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,14 @@ message GetConfiguredNamespacesAndTablesInRSGroupResponse {
repeated TableName table_name = 2;
}

message RenameRSGroupRequest {
required string old_rsgroup_name = 1;
required string new_rsgroup_name = 2;
}

message RenameRSGroupResponse {
}

service RSGroupAdminService {
rpc GetRSGroupInfo(GetRSGroupInfoRequest)
returns (GetRSGroupInfoResponse);
Expand Down Expand Up @@ -172,4 +180,7 @@ service RSGroupAdminService {

rpc RemoveServers(RemoveServersRequest)
returns (RemoveServersResponse);

rpc RenameRSGroup(RenameRSGroupRequest)
returns (RenameRSGroupResponse);
}
18 changes: 18 additions & 0 deletions hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/MasterObserver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1340,6 +1340,24 @@ default void preListTablesInRSGroup(final ObserverContext<MasterCoprocessorEnvir
default void postListTablesInRSGroup(final ObserverContext<MasterCoprocessorEnvironment> ctx,
final String groupName) throws IOException {}

/**
* Called before rename rsgroup.
* @param ctx the environment to interact with the framework and master
* @param oldName old rsgroup name
* @param newName new rsgroup name
*/
default void preRenameRSGroup(final ObserverContext<MasterCoprocessorEnvironment> ctx,
final String oldName, final String newName) throws IOException {}

/**
* Called after rename rsgroup.
* @param ctx the environment to interact with the framework and master
* @param oldName old rsgroup name
* @param newName new rsgroup name
*/
default void postRenameRSGroup(final ObserverContext<MasterCoprocessorEnvironment> ctx,
final String oldName, final String newName) throws IOException {}

/**
* Called before getting the configured namespaces and tables in the region server group.
* @param ctx the environment to interact with the framework and master
Expand Down
20 changes: 20 additions & 0 deletions hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterCoprocessorHost.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1542,6 +1542,26 @@ protected void call(MasterObserver observer) throws IOException {
});
}

public void preRenameRSGroup(final String oldName, final String newName) throws IOException {
execOperation(coprocEnvironments.isEmpty() ? null : new MasterObserverOperation() {

@Override
protected void call(MasterObserver observer) throws IOException {
observer.preRenameRSGroup(this, oldName, newName);
}
});
}

public void postRenameRSGroup(final String oldName, final String newName) throws IOException {
execOperation(coprocEnvironments.isEmpty() ? null : new MasterObserverOperation() {

@Override
protected void call(MasterObserver observer) throws IOException {
observer.postRenameRSGroup(this, oldName, newName);
}
});
}

public void preGetConfiguredNamespacesAndTablesInRSGroup(final String groupName)
throws IOException {
execOperation(coprocEnvironments.isEmpty() ? null : new MasterObserverOperation() {
Expand Down
26 changes: 25 additions & 1 deletion hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterRpcServices.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -341,6 +341,8 @@
import org.apache.hadoop.hbase.shaded.protobuf.generated.RSGroupAdminProtos.RemoveRSGroupResponse;
import org.apache.hadoop.hbase.shaded.protobuf.generated.RSGroupAdminProtos.RemoveServersRequest;
import org.apache.hadoop.hbase.shaded.protobuf.generated.RSGroupAdminProtos.RemoveServersResponse;
import org.apache.hadoop.hbase.shaded.protobuf.generated.RSGroupAdminProtos.RenameRSGroupRequest;
import org.apache.hadoop.hbase.shaded.protobuf.generated.RSGroupAdminProtos.RenameRSGroupResponse;
import org.apache.hadoop.hbase.shaded.protobuf.generated.RegionServerStatusProtos.FileArchiveNotificationRequest;
import org.apache.hadoop.hbase.shaded.protobuf.generated.RegionServerStatusProtos.FileArchiveNotificationResponse;
import org.apache.hadoop.hbase.shaded.protobuf.generated.RegionServerStatusProtos.GetLastFlushedSequenceIdRequest;
Expand Down Expand Up @@ -2456,7 +2458,7 @@ public FileArchiveNotificationResponse reportFileArchival(RpcController controll
throw new ServiceException(e);
}
}

// HBCK Services

@Override
Expand Down Expand Up @@ -3218,4 +3220,26 @@ public ListTablesInRSGroupResponse listTablesInRSGroup(RpcController controller,
}
return builder.build();
}

@Override
public RenameRSGroupResponse renameRSGroup(RpcController controller,
RenameRSGroupRequest request) throws ServiceException {
RenameRSGroupResponse.Builder builder = RenameRSGroupResponse.newBuilder();
String oldRSGroup = request.getOldRsgroupName();
String newRSGroup = request.getNewRsgroupName();
LOG.info("{} rename rsgroup from {} to {} ",
master.getClientIdAuditPrefix(), oldRSGroup, newRSGroup);
try {
if (master.getMasterCoprocessorHost() != null) {
master.getMasterCoprocessorHost().preRenameRSGroup(oldRSGroup, newRSGroup);
}
master.getRSGroupInfoManager().renameRSGroup(oldRSGroup, newRSGroup);
if (master.getMasterCoprocessorHost() != null) {
master.getMasterCoprocessorHost().postRenameRSGroup(oldRSGroup, newRSGroup);
}
} catch (IOException e) {
throw new ServiceException(e);
}
return builder.build();
}
}
4 changes: 4 additions & 0 deletions hbase-server/src/main/java/org/apache/hadoop/hbase/rsgroup/DisabledRSGroupInfoManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,4 +121,8 @@ public String determineRSGroupInfoForTable(TableName tableName) {
return RSGroupInfo.DEFAULT_GROUP;
}

@Override
public void renameRSGroup(String oldName, String newName) throws IOException {
}

}
27 changes: 26 additions & 1 deletion hbase-server/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupAdminServiceImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,8 @@
import org.apache.hadoop.hbase.shaded.protobuf.generated.RSGroupAdminProtos.RemoveRSGroupResponse;
import org.apache.hadoop.hbase.shaded.protobuf.generated.RSGroupAdminProtos.RemoveServersRequest;
import org.apache.hadoop.hbase.shaded.protobuf.generated.RSGroupAdminProtos.RemoveServersResponse;
import org.apache.hadoop.hbase.shaded.protobuf.generated.RSGroupAdminProtos.RenameRSGroupRequest;
import org.apache.hadoop.hbase.shaded.protobuf.generated.RSGroupAdminProtos.RenameRSGroupResponse;

/**
* Implementation of RSGroupAdminService defined in RSGroupAdmin.proto. This class calls
Expand Down Expand Up @@ -396,4 +398,27 @@ public void removeServers(RpcController controller, RemoveServersRequest request
done.run(builder.build());
}

}
@Override
public void renameRSGroup(RpcController controller, RenameRSGroupRequest request,
RpcCallback<RenameRSGroupResponse> done) {
String oldRSGroup = request.getOldRsgroupName();
String newRSGroup = request.getNewRsgroupName();
LOG.info("{} rename rsgroup from {} to {}",
master.getClientIdAuditPrefix(), oldRSGroup, newRSGroup);

RenameRSGroupResponse.Builder builder = RenameRSGroupResponse.newBuilder();
try {
if (master.getMasterCoprocessorHost() != null) {
master.getMasterCoprocessorHost().preRenameRSGroup(oldRSGroup, newRSGroup);
}
rsGroupInfoManager.renameRSGroup(oldRSGroup, newRSGroup);
if (master.getMasterCoprocessorHost() != null) {
master.getMasterCoprocessorHost().postRenameRSGroup(oldRSGroup, newRSGroup);
}
} catch (IOException e) {
CoprocessorRpcUtils.setControllerException(controller, e);
}
done.run(builder.build());
}

}
8 changes: 8 additions & 0 deletions hbase-server/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupInfoManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,4 +106,12 @@ static RSGroupInfoManager create(MasterServices master) throws IOException {
*/
String determineRSGroupInfoForTable(TableName tableName);

/**
* Rename rsgroup
* @param oldName old rsgroup name
* @param newName new rsgroup name
* @throws IOException
*/
void renameRSGroup(String oldName, String newName) throws IOException;

}
29 changes: 29 additions & 0 deletions hbase-server/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupInfoManagerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1221,4 +1221,33 @@ public String determineRSGroupInfoForTable(TableName tableName) {
return script.getRSGroup(tableName.getNamespaceAsString(), tableName.getQualifierAsString());
}

@Override
public synchronized void renameRSGroup(String oldName, String newName) throws IOException {
if (oldName.equals(RSGroupInfo.DEFAULT_GROUP)) {
throw new ConstraintException(RSGroupInfo.DEFAULT_GROUP + " can't be rename");
}
checkGroupName(newName);

RSGroupInfo oldRSG = getRSGroupInfo(oldName);
Map<String, RSGroupInfo> rsGroupMap = holder.groupName2Group;
Map<String, RSGroupInfo> newGroupMap = Maps.newHashMap(rsGroupMap);
newGroupMap.remove(oldRSG.getName());
RSGroupInfo newRSG = new RSGroupInfo(newName, oldRSG.getServers());
newGroupMap.put(newName, newRSG);
flushConfig(newGroupMap);

TableDescriptors tableDescriptors = masterServices.getTableDescriptors();
Set<TableName> updateTables = new HashSet<>();
for (Map.Entry<String, TableDescriptor> table : tableDescriptors.getAll().entrySet()) {
Optional<String> rsgroup = table.getValue().getRegionServerGroup();
if (!rsgroup.isPresent()) {
continue;
}
if (rsgroup.get().equals(oldName)) {
updateTables.add(table.getValue().getTableName());
}
}
setRSGroup(updateTables, newName);
Apache9 marked this conversation as resolved.
Show resolved Hide resolved
}

}
29 changes: 29 additions & 0 deletions hbase-server/src/test/java/org/apache/hadoop/hbase/rsgroup/TestRSGroupsAdmin1.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -468,4 +468,33 @@ public boolean evaluate() throws Exception {
// Cleanup
TEST_UTIL.deleteTable(tn1);
}

@Test
public void testRenameRSGroup() throws Exception {
// Add rsgroup, and assign 2 servers and a table to it.
RSGroupInfo oldgroup = addGroup("oldgroup", 2);
TableName tb = TableName.valueOf("testRename");
TEST_UTIL.createTable(tb, "tr");
ADMIN.setRSGroup(Sets.newHashSet(tb), oldgroup.getName());
TEST_UTIL.waitFor(1000,
(Waiter.Predicate<Exception>) () ->
ADMIN.getRSGroup("oldgroup").getServers().size() == 2);
oldgroup = ADMIN.getRSGroup(oldgroup.getName());
assertEquals(2, oldgroup.getServers().size());
assertEquals(oldgroup.getName(), ADMIN.getRSGroup(tb).getName());

// Rename rsgroup
ADMIN.renameRSGroup(oldgroup.getName(), "newgroup");
Set<Address> servers = oldgroup.getServers();
RSGroupInfo newgroup = ADMIN.getRSGroup("newgroup");
assertEquals(servers.size(), newgroup.getServers().size());
int match = 0;
for (Address addr : newgroup.getServers()) {
if (servers.contains(addr)) {
match++;
}
}
assertEquals(servers.size(), match);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this redundant assert? assertEquals(servers.size(), newgroup.getServers().size()); is already taking care of it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The for loop above is to check the servers are exactly the same before renaming. And the match is the number of 「exactly the same」. Not redundant regarding to its purpose.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I agree this is useful assertion.

Actually I was wondering if we can add more context to this test as well as assertions. For example, we create and assign another table to another rsgroup. So the "if" clause checking rsgroup name in RSGroupInfoManagerImpl.java will get tested. Otherwise this test may still pass even when we remove the "if". By the "if" I mean:

if (rsgroup.get().equals(oldName)) {
    updateTables.add(table.getValue().getTableName());
  }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Get your idea. Fixed in new commit.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes the test now is very comprehensive. Thanks,

assertEquals(newgroup.getName(), ADMIN.getRSGroup(tb).getName());
}
}
6 changes: 6 additions & 0 deletions hbase-server/src/test/java/org/apache/hadoop/hbase/rsgroup/VerifyingRSGroupAdmin.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -816,6 +816,12 @@ public boolean balanceRSGroup(String groupName) throws IOException {
return admin.balanceRSGroup(groupName);
}

@Override
public void renameRSGroup(String oldName, String newName) throws IOException {
admin.renameRSGroup(oldName, newName);
verify();
}

private void verify() throws IOException {
Map<String, RSGroupInfo> groupMap = Maps.newHashMap();
Set<RSGroupInfo> zList = Sets.newHashSet();
Expand Down
4 changes: 4 additions & 0 deletions hbase-thrift/src/main/java/org/apache/hadoop/hbase/thrift2/client/ThriftAdmin.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1257,4 +1257,8 @@ public List<TableName> listTablesInRSGroup(String groupName) throws IOException
throw new NotImplementedException("setRSGroup not supported in ThriftAdmin");
}

@Override
public void renameRSGroup(String oldName, String newName) throws IOException {
throw new NotImplementedException("renameRSGroup not supported in ThriftAdmin");
}
}