saintstack commented on a change in pull request #1482:
URL: https://github.com/apache/hbase/pull/1482#discussion_r416139353
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/HDFSBlocksDistribution.java
##########
@@ -199,10 +225,23 @@ public long getUniqueBlocksTotalWeight() {
* @return the locality index of the given host
*/
public float getBlockLocalityIndex(String host) {
+ return getBlockLocalityIndex(host, false);
+ }
+ /**
+ * return the locality index of a given host
+ * @param host the host name
+ * @param forSsd only calc ssd type
+ * @return the locality index of the given host
+ */
+ public float getBlockLocalityIndex(String host, boolean forSsd) {
float localityIndex = 0;
HostAndWeight hostAndWeight = this.hostAndWeights.get(host);
if (hostAndWeight != null && uniqueBlocksTotalWeight != 0) {
- localityIndex=(float)hostAndWeight.weight/(float)uniqueBlocksTotalWeight;
+ if (forSsd){
+
localityIndex=(float)hostAndWeight.weightForSsd/(float)uniqueBlocksTotalWeight;
+ }else {
Review comment:
Please use style of the surrounding code. See how there is a space
between ){ and between }else.... Also an empty line between methods.
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/HDFSBlocksDistribution.java
##########
@@ -218,7 +257,7 @@ public void add(HDFSBlocksDistribution
otherBlocksDistribution) {
for (Map.Entry<String, HostAndWeight> otherHostAndWeight:
otherHostAndWeights.entrySet()) {
addHostAndBlockWeight(otherHostAndWeight.getValue().host,
- otherHostAndWeight.getValue().weight);
+
otherHostAndWeight.getValue().weight,otherHostAndWeight.getValue().weightForSsd);
Review comment:
And space after ','.
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/master/balancer/StochasticLoadBalancer.java
##########
@@ -1105,6 +1114,28 @@ int regionIndexToEntityIndex(int region) {
}
}
+ static class ServerSsdLocalityCostFunction extends LocalityBasedCostFunction
{
Review comment:
What does this function do? Favor SSD? Needs comment.
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/master/balancer/StochasticLoadBalancer.java
##########
@@ -1058,7 +1065,7 @@ private int getMostLocalEntityForRegion(int region) {
}
private double getWeightedLocality(int region, int entity) {
- return cluster.getOrComputeWeightedLocality(region, entity, type);
+ return cluster.getOrComputeWeightedLocality(region, entity, type,
onlyOnSsd);
Review comment:
Rather than a boolean for ssd, should this be a type? Maybe we want to
favor non-ssd in another balancer impl?
##########
File path:
hbase-server/src/test/java/org/apache/hadoop/hbase/master/balancer/TestStochasticLoadBalancer.java
##########
@@ -511,7 +511,7 @@ public MockCluster(int[][] regions) {
}
@Override
- float getLocalityOfRegion(int region, int server) {
+ float getLocalityOfRegion(int region, int server, boolean onlyOnSsd) {
// convert the locality percentage to a fraction
return localities[region][server] / 100.0f;
}
Review comment:
Needs test to demonstrate this new attribute works? Has an effect?
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/master/balancer/StochasticLoadBalancer.java
##########
@@ -1105,6 +1114,28 @@ int regionIndexToEntityIndex(int region) {
}
}
+ static class ServerSsdLocalityCostFunction extends LocalityBasedCostFunction
{
+
+ private static final String LOCALITY_COST_KEY =
"hbase.master.balancer.stochastic.ssdLocalityCost";
+ private static final float DEFAULT_LOCALITY_COST = 25;
+
+ ServerSsdLocalityCostFunction(Configuration conf, MasterServices srv) {
+ super(
+ conf,
+ srv,
+ LocalityType.SERVER,
+ true,
+ LOCALITY_COST_KEY,
+ DEFAULT_LOCALITY_COST
+ );
Review comment:
Please use style of surrounding code. Params are on one line, not a line
per param.
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/master/balancer/StochasticLoadBalancer.java
##########
@@ -986,6 +990,7 @@ protected double cost() {
static abstract class LocalityBasedCostFunction extends CostFunction {
private final LocalityType type;
+ private final boolean onlyOnSsd;
Review comment:
Should this be a favored type instead of a boolean?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
