You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@hbase.apache.org by GitBox <gi...@apache.org> on 2021/08/11 04:06:31 UTC
[GitHub] [hbase] saintstack commented on a change in pull request #3575: HBASE-26178 Improve data structure and algorithm for BalanceClusterSt…
saintstack commented on a change in pull request #3575:
URL: https://github.com/apache/hbase/pull/3575#discussion_r686471173
##########
File path: hbase-balancer/src/main/java/org/apache/hadoop/hbase/master/balancer/CandidateGenerator.java
##########
@@ -42,13 +42,13 @@
*/
int pickRandomRegion(BalancerClusterState cluster, int server, double chanceOfNoSwap) {
// Check to see if this is just a move.
- if (cluster.regionsPerServer[server].length == 0
+ if (cluster.regionsPerServer.get(server).size() == 0
Review comment:
s/isEmpty()/size() == 0/ ... its faster.
##########
File path: hbase-balancer/src/main/java/org/apache/hadoop/hbase/master/balancer/RegionReplicaGroupingCostFunction.java
##########
@@ -44,11 +46,12 @@ final void prepare(BalancerClusterState cluster) {
protected final long getMaxCost(BalancerClusterState cluster) {
// max cost is the case where every region replica is hosted together regardless of host
- int[] primariesOfRegions = new int[cluster.numRegions];
- System.arraycopy(cluster.regionIndexToPrimaryIndex, 0, primariesOfRegions, 0,
- cluster.regions.length);
-
- Arrays.sort(primariesOfRegions);
+ HashMap<Integer, ArrayList<Integer>> primariesOfRegions =
+ new HashMap<Integer, ArrayList<Integer>>(cluster.numRegions);
Review comment:
We don't do this double allocation on a single line... we give each allocation and assign its own line... easier to read.
Left hand size should be Map rather than HashMap? Ditto for List and ArrayLIst.
##########
File path: hbase-balancer/src/main/java/org/apache/hadoop/hbase/master/balancer/RegionReplicaGroupingCostFunction.java
##########
@@ -80,22 +83,16 @@ protected double cost() {
* @param primariesOfRegions a sorted array of primary regions ids for the regions hosted
* @return a sum of numReplicas-1 squared for each primary region in the group.
*/
- protected final long costPerGroup(int[] primariesOfRegions) {
+ protected final long costPerGroup(HashMap<Integer, ArrayList<Integer>> primariesOfRegions) {
Review comment:
s/Map/HashMap/ and s/List/ArrayList/ ?
##########
File path: hbase-balancer/src/main/java/org/apache/hadoop/hbase/master/balancer/RegionReplicaCandidateGenerator.java
##########
@@ -39,42 +44,37 @@
* @param regionIndexToPrimaryIndex Cluster.regionsIndexToPrimaryIndex
* @return a regionIndex for the selected primary or -1 if there is no co-locating
*/
- int selectCoHostedRegionPerGroup(int[] primariesOfRegionsPerGroup, int[] regionsPerGroup,
- int[] regionIndexToPrimaryIndex) {
- int currentPrimary = -1;
- int currentPrimaryIndex = -1;
- int selectedPrimaryIndex = -1;
+ int selectCoHostedRegionPerGroup(HashMap<Integer, ArrayList<Integer>> primariesOfRegionsPerGroup,
Review comment:
s/List/ArrayList/ and s/Map/HashMap/?
##########
File path: hbase-balancer/src/main/java/org/apache/hadoop/hbase/master/balancer/RegionReplicaHostCostFunction.java
##########
@@ -32,7 +35,7 @@
"hbase.master.balancer.stochastic.regionReplicaHostCostKey";
private static final float DEFAULT_REGION_REPLICA_HOST_COST_KEY = 100000;
- private int[][] primariesOfRegionsPerGroup;
+ private ArrayList<HashMap<Integer, ArrayList<Integer>>> primariesOfRegionsPerGroup;
Review comment:
s/Map/HashMap/
##########
File path: hbase-balancer/src/main/java/org/apache/hadoop/hbase/master/balancer/BaseLoadBalancer.java
##########
@@ -128,11 +128,11 @@ public void postMasterStartupInitialize() {
protected final boolean idleRegionServerExist(BalancerClusterState c){
boolean isServerExistsWithMoreRegions = false;
boolean isServerExistsWithZeroRegions = false;
- for (int[] serverList: c.regionsPerServer){
- if (serverList.length > 1) {
+ for (ArrayList<Integer> serverList: c.regionsPerServer){
Review comment:
s/List/ArrayList/ ?
##########
File path: hbase-balancer/src/main/java/org/apache/hadoop/hbase/master/balancer/RegionReplicaHostCostFunction.java
##########
@@ -32,7 +35,7 @@
"hbase.master.balancer.stochastic.regionReplicaHostCostKey";
private static final float DEFAULT_REGION_REPLICA_HOST_COST_KEY = 100000;
- private int[][] primariesOfRegionsPerGroup;
+ private ArrayList<HashMap<Integer, ArrayList<Integer>>> primariesOfRegionsPerGroup;
Review comment:
What is the advantage of List of int [] ?
s/List/ArrayList/ ?
##########
File path: hbase-balancer/src/main/java/org/apache/hadoop/hbase/master/balancer/RegionReplicaCandidateGenerator.java
##########
@@ -39,42 +44,37 @@
* @param regionIndexToPrimaryIndex Cluster.regionsIndexToPrimaryIndex
* @return a regionIndex for the selected primary or -1 if there is no co-locating
*/
- int selectCoHostedRegionPerGroup(int[] primariesOfRegionsPerGroup, int[] regionsPerGroup,
- int[] regionIndexToPrimaryIndex) {
- int currentPrimary = -1;
- int currentPrimaryIndex = -1;
- int selectedPrimaryIndex = -1;
+ int selectCoHostedRegionPerGroup(HashMap<Integer, ArrayList<Integer>> primariesOfRegionsPerGroup,
+ Collection<Integer> regionsPerGroup, int[] regionIndexToPrimaryIndex) {
+
double currentLargestRandom = -1;
- // primariesOfRegionsPerGroup is a sorted array. Since it contains the primary region
- // ids for the regions hosted in server, a consecutive repetition means that replicas
- // are co-hosted
- for (int j = 0; j <= primariesOfRegionsPerGroup.length; j++) {
- int primary = j < primariesOfRegionsPerGroup.length ? primariesOfRegionsPerGroup[j] : -1;
- if (primary != currentPrimary) { // check for whether we see a new primary
- int numReplicas = j - currentPrimaryIndex;
- if (numReplicas > 1) { // means consecutive primaries, indicating co-location
- // decide to select this primary region id or not
- double currentRandom = ThreadLocalRandom.current().nextDouble();
- // we don't know how many region replicas are co-hosted, we will randomly select one
- // using reservoir sampling (http://gregable.com/2007/10/reservoir-sampling.html)
- if (currentRandom > currentLargestRandom) {
- selectedPrimaryIndex = currentPrimary;
- currentLargestRandom = currentRandom;
- }
+ Map.Entry<Integer, ArrayList<Integer>> selectedPrimaryIndexEntry = null;
+
+ // primariesOfRegionsPerGroup is a hashmap of count of primary index on a server. a count > 1
+ // means that replicas are co-hosted
+ for(Map.Entry<Integer, ArrayList<Integer>> pair : primariesOfRegionsPerGroup.entrySet()) {
Review comment:
s/LIst/ArrayList/ unless you are using explicit ArrayList methods?
##########
File path: hbase-balancer/src/main/java/org/apache/hadoop/hbase/master/balancer/FavoredStochasticBalancer.java
##########
@@ -526,9 +526,9 @@ protected BalanceAction generate(BalancerClusterState cluster) {
thisRegion = pickLowestLocalRegionOnServer(cluster, thisServer);
}
if (thisRegion == -1) {
- if (cluster.regionsPerServer[thisServer].length > 0) {
+ if (cluster.regionsPerServer.get(thisServer).size() > 0) {
Review comment:
s/isEmpty()/size()/ and negate the check?
##########
File path: hbase-balancer/src/main/java/org/apache/hadoop/hbase/master/balancer/RegionReplicaGroupingCostFunction.java
##########
@@ -80,22 +83,16 @@ protected double cost() {
* @param primariesOfRegions a sorted array of primary regions ids for the regions hosted
* @return a sum of numReplicas-1 squared for each primary region in the group.
*/
- protected final long costPerGroup(int[] primariesOfRegions) {
+ protected final long costPerGroup(HashMap<Integer, ArrayList<Integer>> primariesOfRegions) {
long cost = 0;
- int currentPrimary = -1;
- int currentPrimaryIndex = -1;
- // primariesOfRegions is a sorted array of primary ids of regions. Replicas of regions
- // sharing the same primary will have consecutive numbers in the array.
- for (int j = 0; j <= primariesOfRegions.length; j++) {
- int primary = j < primariesOfRegions.length ? primariesOfRegions[j] : -1;
- if (primary != currentPrimary) { // we see a new primary
- int numReplicas = j - currentPrimaryIndex;
- // square the cost
- if (numReplicas > 1) { // means consecutive primaries, indicating co-location
- cost += (numReplicas - 1) * (numReplicas - 1);
- }
- currentPrimary = primary;
- currentPrimaryIndex = j;
+
+ // primariesOfRegionsPerGroup is a hashmap of count of primary index on a server. a count > 1
+ // means that replicas are co-hosted
+ for(Map.Entry<Integer, ArrayList<Integer>> pair : primariesOfRegions.entrySet()) {
Review comment:
s/List/ArrayList/
##########
File path: hbase-balancer/src/main/java/org/apache/hadoop/hbase/master/balancer/RegionReplicaCandidateGenerator.java
##########
@@ -39,42 +44,37 @@
* @param regionIndexToPrimaryIndex Cluster.regionsIndexToPrimaryIndex
* @return a regionIndex for the selected primary or -1 if there is no co-locating
*/
- int selectCoHostedRegionPerGroup(int[] primariesOfRegionsPerGroup, int[] regionsPerGroup,
- int[] regionIndexToPrimaryIndex) {
- int currentPrimary = -1;
- int currentPrimaryIndex = -1;
- int selectedPrimaryIndex = -1;
+ int selectCoHostedRegionPerGroup(HashMap<Integer, ArrayList<Integer>> primariesOfRegionsPerGroup,
+ Collection<Integer> regionsPerGroup, int[] regionIndexToPrimaryIndex) {
+
double currentLargestRandom = -1;
- // primariesOfRegionsPerGroup is a sorted array. Since it contains the primary region
- // ids for the regions hosted in server, a consecutive repetition means that replicas
- // are co-hosted
- for (int j = 0; j <= primariesOfRegionsPerGroup.length; j++) {
- int primary = j < primariesOfRegionsPerGroup.length ? primariesOfRegionsPerGroup[j] : -1;
- if (primary != currentPrimary) { // check for whether we see a new primary
- int numReplicas = j - currentPrimaryIndex;
- if (numReplicas > 1) { // means consecutive primaries, indicating co-location
- // decide to select this primary region id or not
- double currentRandom = ThreadLocalRandom.current().nextDouble();
- // we don't know how many region replicas are co-hosted, we will randomly select one
- // using reservoir sampling (http://gregable.com/2007/10/reservoir-sampling.html)
- if (currentRandom > currentLargestRandom) {
- selectedPrimaryIndex = currentPrimary;
- currentLargestRandom = currentRandom;
- }
+ Map.Entry<Integer, ArrayList<Integer>> selectedPrimaryIndexEntry = null;
Review comment:
s/List/ArrayList/ ?
##########
File path: hbase-balancer/src/main/java/org/apache/hadoop/hbase/master/balancer/RegionReplicaCandidateGenerator.java
##########
@@ -39,42 +44,37 @@
* @param regionIndexToPrimaryIndex Cluster.regionsIndexToPrimaryIndex
* @return a regionIndex for the selected primary or -1 if there is no co-locating
*/
- int selectCoHostedRegionPerGroup(int[] primariesOfRegionsPerGroup, int[] regionsPerGroup,
- int[] regionIndexToPrimaryIndex) {
- int currentPrimary = -1;
- int currentPrimaryIndex = -1;
- int selectedPrimaryIndex = -1;
+ int selectCoHostedRegionPerGroup(HashMap<Integer, ArrayList<Integer>> primariesOfRegionsPerGroup,
+ Collection<Integer> regionsPerGroup, int[] regionIndexToPrimaryIndex) {
+
double currentLargestRandom = -1;
- // primariesOfRegionsPerGroup is a sorted array. Since it contains the primary region
- // ids for the regions hosted in server, a consecutive repetition means that replicas
- // are co-hosted
- for (int j = 0; j <= primariesOfRegionsPerGroup.length; j++) {
- int primary = j < primariesOfRegionsPerGroup.length ? primariesOfRegionsPerGroup[j] : -1;
- if (primary != currentPrimary) { // check for whether we see a new primary
- int numReplicas = j - currentPrimaryIndex;
- if (numReplicas > 1) { // means consecutive primaries, indicating co-location
- // decide to select this primary region id or not
- double currentRandom = ThreadLocalRandom.current().nextDouble();
- // we don't know how many region replicas are co-hosted, we will randomly select one
- // using reservoir sampling (http://gregable.com/2007/10/reservoir-sampling.html)
- if (currentRandom > currentLargestRandom) {
- selectedPrimaryIndex = currentPrimary;
- currentLargestRandom = currentRandom;
- }
+ Map.Entry<Integer, ArrayList<Integer>> selectedPrimaryIndexEntry = null;
+
+ // primariesOfRegionsPerGroup is a hashmap of count of primary index on a server. a count > 1
Review comment:
For my info, whats a 'count of primary index on a server'? Thanks.
--
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: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org