ndimiduk commented on a change in pull request #3536:
URL: https://github.com/apache/hbase/pull/3536#discussion_r686146256
##########
File path:
hbase-rsgroup/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupAdminServer.java
##########
@@ -541,6 +544,11 @@ public boolean balanceRSGroup(String groupName) throws
IOException {
getRSGroupAssignmentsByTable(master.getTableStateManager(),
groupName);
List<RegionPlan> plans = balancer.balanceCluster(assignmentsByTable);
boolean balancerRan = !plans.isEmpty();
+
+ if (request.isDryRun()) {
+ return balancerRan;
Review comment:
I was anticipating a discussion, not code changes. I'm fine with leaving
it as a boolean response, I just wanted to make sure we think about what the
response values mean, and we document them.
There should already be a `BalancerResponse` in the protobuf, so there's a
mechanism in place for adding a more rich response to the client experience. If
we're adding to it, why not also include the amount of time spent on each
calculation and execution of moves? I agree, it would be easy to add fields,
and I'm not clear on the value they might provide.
##########
File path:
hbase-rsgroup/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupAdminEndpoint.java
##########
@@ -291,17 +292,19 @@ public void removeRSGroup(RpcController controller,
@Override
public void balanceRSGroup(RpcController controller,
BalanceRSGroupRequest request, RpcCallback<BalanceRSGroupResponse>
done) {
+ BalanceRequest balanceRequest =
RSGroupProtobufUtil.toBalanceRequest(request);
+
BalanceRSGroupResponse.Builder builder =
BalanceRSGroupResponse.newBuilder();
LOG.info(master.getClientIdAuditPrefix() + " balance rsgroup, group="
+ request.getRSGroupName());
try {
- if (master.getMasterCoprocessorHost() != null) {
+ if (master.getMasterCoprocessorHost() != null &&
!balanceRequest.isDryRun()) {
Review comment:
As coprocessors go, I think it's not up HBase to decide what behavior a
coprocessor implements, only to faithfully delegate calls at the defined
extension points. I think the dry_run mode should pass through any installed
coprocessors, which means this change will require a change to coprocessor
APIs. This falls more explicitly under the terms of HBase LimitedPrivate API,
which means it will require a minor version bump. But it's a change to the
public API, so I assume a minor version bump was required anyway.
If you're concerned that making this change is too meddlesome for this
patch, I'm okay with a follow-on jira being filed and a TODO being left in the
code here.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
