HBASE-23949 refactor loadBalancer implements for rsgroup balance by t… #1279
Conversation
This file contains 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
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
infraio
reviewed
Mar 12, 2020
| @@ -89,18 +89,18 @@ | |||
| void setMasterServices(MasterServices masterServices); | |||
|
|
|||
| /** | |||
| * Perform the major balance operation | |||
| * Perform the major balance operation for cluster | |||
| * @return List of plans | |||
| */ | |||
There was a problem hiding this comment.
Add javadoc for the parameter of balanceCluster and balanceTable?
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
infraio
reviewed
Mar 12, 2020
| } | ||
|
|
||
| @Override | ||
| public synchronized List<RegionPlan> |
There was a problem hiding this comment.
method of the extends class need be synchronized
infraio
reviewed
Mar 12, 2020
| } | ||
|
|
||
| @Override | ||
| public List<RegionPlan> balanceTable(TableName tableName, Map<ServerName, List<RegionInfo>> clusterLoad){ |
There was a problem hiding this comment.
Unsupport? Should be abstrat method?
There was a problem hiding this comment.
yes ,it should be abstrat method
infraio
reviewed
Mar 12, 2020
| @@ -106,6 +106,12 @@ void setNextRegionForUnload(int nextRegionForUnload) { | |||
|
|
|||
| } | |||
|
|
|||
| @Override | |||
| public synchronized void setConf(Configuration conf) { | |||
There was a problem hiding this comment.
yes, I remove it
infraio
reviewed
Mar 12, 2020
| return balanceCluster(clusterState); | ||
| public synchronized List<RegionPlan> balanceCluster(Map<TableName, Map<ServerName, | ||
| List<RegionInfo>>> clusterLoadPerTable) { | ||
| return super.balanceCluster(clusterLoadPerTable); |
There was a problem hiding this comment.
If call super method directly, no need to add this method here?
There was a problem hiding this comment.
yes, I remove it
infraio
reviewed
Mar 12, 2020
| @@ -427,4 +419,9 @@ public void postMasterStartupInitialize() { | |||
| public void updateBalancerStatus(boolean status) { | |||
| internalBalancer.updateBalancerStatus(status); | |||
| } | |||
|
|
|||
| @Override | |||
| public List<RegionPlan> balanceTable(TableName tableName, Map<ServerName, List<RegionInfo>> clusterLoad){ | |||
There was a problem hiding this comment.
yes , it should support
infraio
reviewed
Mar 12, 2020
| @@ -87,7 +87,8 @@ public synchronized void initialize() throws HBaseIOException { | |||
| } | |||
|
|
|||
| @Override | |||
| public List<RegionPlan> balanceCluster(Map<ServerName, List<RegionInfo>> clusterState) { | |||
| public List<RegionPlan> | |||
| balanceCluster(Map<TableName, Map<ServerName, List<RegionInfo>>> clusterLoadPerTable) { | |||
There was a problem hiding this comment.
The balancer should only need to override balanceTable method?
There was a problem hiding this comment.
yes , but method 'balanceCluster' of this class not invoke 'balanceTable' , so just override balanceTable with null return
There was a problem hiding this comment.
Didn't get your point. Please add javadoc comment about why retrun null here.
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
infraio
reviewed
Mar 16, 2020
| return balanceCluster(clusterState); | ||
| public synchronized List<RegionPlan> | ||
| balanceCluster(Map<TableName, Map<ServerName, List<RegionInfo>>> clusterLoadPerTable) { | ||
| setClusterLoad(clusterLoadPerTable); |
There was a problem hiding this comment.
Add reason for why need to setClusterLoad?
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
binlijin
reviewed
Mar 20, 2020
| }); | ||
| return result; | ||
| } else { | ||
| LOG.debug("Start Generate Balance plan for table: " + HConstants.ENSEMBLE_TABLE_NAME); |
There was a problem hiding this comment.
There is no Table ENSEMBLE_TABLE_NAME actually, need to change this log?
There was a problem hiding this comment.
it convenient to debug ,balance for Table ENSEMBLE_TABLE_NAME , mean balanceByTable is false, we just consider all region as one table's
There was a problem hiding this comment.
May be good to log ""Start generate balance plan for cluster". No need log the ENSEMBLE_TABLE_NAME. And this log can be info?
There was a problem hiding this comment.
yes, you are right
infraio
reviewed
Mar 21, 2020
| if (isByTable) { | ||
| List<RegionPlan> result = new ArrayList<>(); | ||
| loadOfAllTable.forEach((tableName, loadOfOneTable) -> { | ||
| LOG.debug("Start Generate Balance plan for table: " + tableName); |
There was a problem hiding this comment.
LOG.info("Start generate balance plan for table: {}", tableName);
There was a problem hiding this comment.
ok , I will change this
infraio
reviewed
Mar 21, 2020
| List<RegionPlan> groupPlans = this.internalBalancer | ||
| .balanceCluster(groupClusterState); | ||
| List<RegionPlan> groupPlans = null; | ||
| if (!loadOfTablesInGroup.isEmpty()) { |
There was a problem hiding this comment.
well , I add some log
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
…able to achieve overallbalanced
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
thangTang
pushed a commit
to thangTang/hbase
that referenced
this pull request
Apr 16, 2020
…able to achieve overallbalanced (apache#1279) Signed-off-by: Guanghao Zhang <zghao@apache.org>
thangTang
pushed a commit
to thangTang/hbase
that referenced
this pull request
Apr 16, 2020
…able to achieve overallbalanced (apache#1279) Signed-off-by: Guanghao Zhang <zghao@apache.org>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
…able