wchevreuil commented on code in PR #4799:
URL: https://github.com/apache/hbase/pull/4799#discussion_r1099092422
##########
hbase-balancer/src/main/java/org/apache/hadoop/hbase/master/balancer/BalancerClusterState.java:
##########
@@ -553,6 +569,108 @@ enum LocalityType {
RACK
}
+ public float getOrComputeWeightedPrefetchRatio(int region, int server) {
+ return getRegionSizeMB(region) * getOrComputeRegionPrefetchRatio(region,
server);
+ }
+
+ protected float getRegionServerPrefetchRatio(int region, int
regionServerIndex) {
+ // Cost this server has from RegionLoad
+ float prefetchRatio = 0.0f;
+
+ // Get the prefetch ratio if the region is currently hosted on this server
+ for (int regionIndex : regionsPerServer[regionServerIndex]) {
+ if (region != regionIndex) {
+ continue;
+ }
+ Deque<BalancerRegionLoad> regionLoadList = regionLoads[regionIndex];
+
+ // The region is currently hosted on this region server. Now, get the
prefetch cache ratio
+ // for this region on this region server
+ prefetchRatio =
+ regionLoadList == null ? 0.0f :
regionLoadList.getLast().getPrefetchCacheRatio();
+
+ return prefetchRatio;
+ }
+
+ // Region is not currently hosted on this server. Check if the region was
prefetched on this
+ // server earlier. This can happen when the server was shutdown and the
cache was persisted.
+ // Seartch using the index name and server name and not the index id and
server id as these ids
+ // may change when a server is marked as dead or a new server is added.
+ String regionNameAsString = regions[region].getRegionNameAsString();
+ String serverNameAsString = servers[regionServerIndex].getServerName();
+ if (
+ historicalRegionServerPrefetchRatio != null
+ && historicalRegionServerPrefetchRatio.containsKey(regionNameAsString)
+ ) {
+ Map<String, Float> serverPrefetchRatio =
+ historicalRegionServerPrefetchRatio.get(regionNameAsString);
+ if (serverPrefetchRatio.containsKey(serverNameAsString)) {
+ prefetchRatio = serverPrefetchRatio.get(serverNameAsString);
+
+ // The old prefetch cache ratio has been accounted for and hence,
clear up this information
+ historicalRegionServerPrefetchRatio.remove(regionNameAsString,
serverPrefetchRatio);
+ }
+ }
+ return prefetchRatio;
+ }
+
+ private void computeRegionServerPrefetchRatio() {
+ regionIndexServerIndexPrefetchRatio = new HashMap<>();
+ regionServerIndexWithBestPrefetchRatio = new int[numRegions];
+
+ for (int region = 0; region < numRegions; region++) {
+ float bestPrefetchRatio = 0.0f;
+ int serverWithBestPrefetchRatio = 0;
+ for (int server = 0; server < numServers; server++) {
+ float prefetchRatio = getRegionServerPrefetchRatio(region, server);
+ if (prefetchRatio > 0.0f || server ==
regionIndexToServerIndex[region]) {
+ // A region with prefetch ratio of 0 on a server means nothing.
Hence, just make a note
+ // of prefetch only if the prefetch ratio is greater than 0.
+ Map<Integer, Integer> tempMap = new HashMap<>();
Review Comment:
Do we really need a map here? Couldn't this be a pair?
##########
hbase-balancer/src/main/java/org/apache/hadoop/hbase/master/balancer/BalancerClusterState.java:
##########
@@ -553,6 +569,108 @@ enum LocalityType {
RACK
}
+ public float getOrComputeWeightedPrefetchRatio(int region, int server) {
+ return getRegionSizeMB(region) * getOrComputeRegionPrefetchRatio(region,
server);
+ }
+
+ protected float getRegionServerPrefetchRatio(int region, int
regionServerIndex) {
+ // Cost this server has from RegionLoad
+ float prefetchRatio = 0.0f;
+
+ // Get the prefetch ratio if the region is currently hosted on this server
+ for (int regionIndex : regionsPerServer[regionServerIndex]) {
+ if (region != regionIndex) {
+ continue;
+ }
+ Deque<BalancerRegionLoad> regionLoadList = regionLoads[regionIndex];
+
+ // The region is currently hosted on this region server. Now, get the
prefetch cache ratio
+ // for this region on this region server
+ prefetchRatio =
+ regionLoadList == null ? 0.0f :
regionLoadList.getLast().getPrefetchCacheRatio();
+
+ return prefetchRatio;
+ }
+
+ // Region is not currently hosted on this server. Check if the region was
prefetched on this
+ // server earlier. This can happen when the server was shutdown and the
cache was persisted.
+ // Seartch using the index name and server name and not the index id and
server id as these ids
+ // may change when a server is marked as dead or a new server is added.
+ String regionNameAsString = regions[region].getRegionNameAsString();
+ String serverNameAsString = servers[regionServerIndex].getServerName();
+ if (
+ historicalRegionServerPrefetchRatio != null
+ && historicalRegionServerPrefetchRatio.containsKey(regionNameAsString)
+ ) {
+ Map<String, Float> serverPrefetchRatio =
+ historicalRegionServerPrefetchRatio.get(regionNameAsString);
+ if (serverPrefetchRatio.containsKey(serverNameAsString)) {
+ prefetchRatio = serverPrefetchRatio.get(serverNameAsString);
+
+ // The old prefetch cache ratio has been accounted for and hence,
clear up this information
+ historicalRegionServerPrefetchRatio.remove(regionNameAsString,
serverPrefetchRatio);
+ }
+ }
+ return prefetchRatio;
Review Comment:
Is this something we can really trust? Because this may not be the real
prefetch ratio anymore, we are relying on region metrics from the past, but
what if this RS has already evicted some of this region's blocks?
##########
hbase-balancer/src/main/java/org/apache/hadoop/hbase/master/balancer/BalancerClusterState.java:
##########
@@ -553,6 +569,108 @@ enum LocalityType {
RACK
}
+ public float getOrComputeWeightedPrefetchRatio(int region, int server) {
+ return getRegionSizeMB(region) * getOrComputeRegionPrefetchRatio(region,
server);
+ }
+
+ protected float getRegionServerPrefetchRatio(int region, int
regionServerIndex) {
+ // Cost this server has from RegionLoad
+ float prefetchRatio = 0.0f;
+
+ // Get the prefetch ratio if the region is currently hosted on this server
+ for (int regionIndex : regionsPerServer[regionServerIndex]) {
+ if (region != regionIndex) {
+ continue;
+ }
+ Deque<BalancerRegionLoad> regionLoadList = regionLoads[regionIndex];
+
+ // The region is currently hosted on this region server. Now, get the
prefetch cache ratio
+ // for this region on this region server
+ prefetchRatio =
+ regionLoadList == null ? 0.0f :
regionLoadList.getLast().getPrefetchCacheRatio();
+
+ return prefetchRatio;
+ }
+
+ // Region is not currently hosted on this server. Check if the region was
prefetched on this
+ // server earlier. This can happen when the server was shutdown and the
cache was persisted.
+ // Seartch using the index name and server name and not the index id and
server id as these ids
+ // may change when a server is marked as dead or a new server is added.
+ String regionNameAsString = regions[region].getRegionNameAsString();
+ String serverNameAsString = servers[regionServerIndex].getServerName();
+ if (
+ historicalRegionServerPrefetchRatio != null
+ && historicalRegionServerPrefetchRatio.containsKey(regionNameAsString)
+ ) {
+ Map<String, Float> serverPrefetchRatio =
+ historicalRegionServerPrefetchRatio.get(regionNameAsString);
+ if (serverPrefetchRatio.containsKey(serverNameAsString)) {
+ prefetchRatio = serverPrefetchRatio.get(serverNameAsString);
+
+ // The old prefetch cache ratio has been accounted for and hence,
clear up this information
+ historicalRegionServerPrefetchRatio.remove(regionNameAsString,
serverPrefetchRatio);
+ }
+ }
+ return prefetchRatio;
+ }
+
+ private void computeRegionServerPrefetchRatio() {
+ regionIndexServerIndexPrefetchRatio = new HashMap<>();
+ regionServerIndexWithBestPrefetchRatio = new int[numRegions];
+
+ for (int region = 0; region < numRegions; region++) {
+ float bestPrefetchRatio = 0.0f;
+ int serverWithBestPrefetchRatio = 0;
+ for (int server = 0; server < numServers; server++) {
+ float prefetchRatio = getRegionServerPrefetchRatio(region, server);
+ if (prefetchRatio > 0.0f || server ==
regionIndexToServerIndex[region]) {
+ // A region with prefetch ratio of 0 on a server means nothing.
Hence, just make a note
+ // of prefetch only if the prefetch ratio is greater than 0.
Review Comment:
I'm not following these comments. Clearly, we only put a prefetch ratio of 0
if it's the server hosting the region. It sounds one more reason to iterate
through the RS level first (see my comments from Sep above).
##########
hbase-balancer/src/main/java/org/apache/hadoop/hbase/master/balancer/PrefetchCacheCostFunction.java:
##########
@@ -0,0 +1,82 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.master.balancer;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hbase.HConstants;
+import org.apache.yetus.audience.InterfaceAudience;
+
+/**
+ * Compute the cost of a potential cluster configuration based on the number
of HFile's already
+ * cached in the bucket cache
+ */
[email protected]
+public class PrefetchCacheCostFunction extends CostFunction {
+ private static final String PREFETCH_CACHE_COST_KEY =
+ "hbase.master.balancer.stochastic.prefetchCacheCost";
+ private static final float DEFAULT_PREFETCH_COST = 500;
+
+ private String prefetchedFileListPath;
+ private double prefetchRatio;
+ private float bestPrefetchRatio;
+
+ PrefetchCacheCostFunction(Configuration conf) {
+ prefetchedFileListPath =
conf.get(HConstants.PREFETCH_PERSISTENCE_PATH_KEY);
+ // Disable the prefetch cache cost function if the prefetched file list
persistence is not
+ // enabled
+ this.setMultiplier(prefetchedFileListPath == null
+ ? 0.0f
+ : conf.getFloat(PREFETCH_CACHE_COST_KEY, DEFAULT_PREFETCH_COST));
+ prefetchRatio = 0.0f;
+ bestPrefetchRatio = 0.0f;
+ }
+
+ @Override
+ void prepare(BalancerClusterState cluster) {
+ super.prepare(cluster);
+ prefetchRatio = 0.0f;
+ bestPrefetchRatio = 0.0f;
+
+ for (int region = 0; region < cluster.numRegions; region++) {
+ prefetchRatio +=
+ cluster.getOrComputeWeightedPrefetchRatio(region,
cluster.regionIndexToServerIndex[region]);
+ bestPrefetchRatio += cluster.getOrComputeWeightedPrefetchRatio(region,
+ cluster.getOrComputeServerWithBestPrefetchRatio()[region]);
+ }
+ prefetchRatio = bestPrefetchRatio == 0.0f ? 1.0f : prefetchRatio /
bestPrefetchRatio;
Review Comment:
I'm a bit confused here. When would prefetchRatio be different from
bestPrefetchRatio?
##########
hbase-balancer/src/main/java/org/apache/hadoop/hbase/master/balancer/PrefetchCacheCostFunction.java:
##########
@@ -0,0 +1,82 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.master.balancer;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hbase.HConstants;
+import org.apache.yetus.audience.InterfaceAudience;
+
+/**
+ * Compute the cost of a potential cluster configuration based on the number
of HFile's already
+ * cached in the bucket cache
+ */
[email protected]
+public class PrefetchCacheCostFunction extends CostFunction {
+ private static final String PREFETCH_CACHE_COST_KEY =
+ "hbase.master.balancer.stochastic.prefetchCacheCost";
+ private static final float DEFAULT_PREFETCH_COST = 500;
+
+ private String prefetchedFileListPath;
+ private double prefetchRatio;
+ private float bestPrefetchRatio;
+
+ PrefetchCacheCostFunction(Configuration conf) {
+ prefetchedFileListPath =
conf.get(HConstants.PREFETCH_PERSISTENCE_PATH_KEY);
+ // Disable the prefetch cache cost function if the prefetched file list
persistence is not
+ // enabled
+ this.setMultiplier(prefetchedFileListPath == null
+ ? 0.0f
+ : conf.getFloat(PREFETCH_CACHE_COST_KEY, DEFAULT_PREFETCH_COST));
+ prefetchRatio = 0.0f;
+ bestPrefetchRatio = 0.0f;
+ }
+
+ @Override
+ void prepare(BalancerClusterState cluster) {
+ super.prepare(cluster);
+ prefetchRatio = 0.0f;
+ bestPrefetchRatio = 0.0f;
+
+ for (int region = 0; region < cluster.numRegions; region++) {
+ prefetchRatio +=
+ cluster.getOrComputeWeightedPrefetchRatio(region,
cluster.regionIndexToServerIndex[region]);
+ bestPrefetchRatio += cluster.getOrComputeWeightedPrefetchRatio(region,
+ cluster.getOrComputeServerWithBestPrefetchRatio()[region]);
+ }
+ prefetchRatio = bestPrefetchRatio == 0.0f ? 1.0f : prefetchRatio /
bestPrefetchRatio;
+ }
+
+ @Override
+ protected double cost() {
+ return 1 - prefetchRatio;
+ }
Review Comment:
So higher prefetch means lower cost. I guess this is to influence the
decision made by StochasticLoadBalancer.needsBalance() method.
##########
hbase-balancer/src/main/java/org/apache/hadoop/hbase/master/balancer/PrefetchCacheCostFunction.java:
##########
@@ -0,0 +1,82 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.master.balancer;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hbase.HConstants;
+import org.apache.yetus.audience.InterfaceAudience;
+
+/**
+ * Compute the cost of a potential cluster configuration based on the number
of HFile's already
+ * cached in the bucket cache
+ */
[email protected]
+public class PrefetchCacheCostFunction extends CostFunction {
+ private static final String PREFETCH_CACHE_COST_KEY =
+ "hbase.master.balancer.stochastic.prefetchCacheCost";
+ private static final float DEFAULT_PREFETCH_COST = 500;
+
+ private String prefetchedFileListPath;
+ private double prefetchRatio;
+ private float bestPrefetchRatio;
+
+ PrefetchCacheCostFunction(Configuration conf) {
+ prefetchedFileListPath =
conf.get(HConstants.PREFETCH_PERSISTENCE_PATH_KEY);
+ // Disable the prefetch cache cost function if the prefetched file list
persistence is not
+ // enabled
+ this.setMultiplier(prefetchedFileListPath == null
Review Comment:
Please explain this in the class javadoc.
##########
hbase-balancer/src/main/java/org/apache/hadoop/hbase/master/balancer/PrefetchCacheCostFunction.java:
##########
@@ -0,0 +1,82 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.master.balancer;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hbase.HConstants;
+import org.apache.yetus.audience.InterfaceAudience;
+
+/**
+ * Compute the cost of a potential cluster configuration based on the number
of HFile's already
+ * cached in the bucket cache
+ */
[email protected]
+public class PrefetchCacheCostFunction extends CostFunction {
+ private static final String PREFETCH_CACHE_COST_KEY =
+ "hbase.master.balancer.stochastic.prefetchCacheCost";
+ private static final float DEFAULT_PREFETCH_COST = 500;
+
+ private String prefetchedFileListPath;
+ private double prefetchRatio;
+ private float bestPrefetchRatio;
+
+ PrefetchCacheCostFunction(Configuration conf) {
+ prefetchedFileListPath =
conf.get(HConstants.PREFETCH_PERSISTENCE_PATH_KEY);
+ // Disable the prefetch cache cost function if the prefetched file list
persistence is not
+ // enabled
+ this.setMultiplier(prefetchedFileListPath == null
+ ? 0.0f
+ : conf.getFloat(PREFETCH_CACHE_COST_KEY, DEFAULT_PREFETCH_COST));
+ prefetchRatio = 0.0f;
+ bestPrefetchRatio = 0.0f;
+ }
+
+ @Override
+ void prepare(BalancerClusterState cluster) {
+ super.prepare(cluster);
+ prefetchRatio = 0.0f;
+ bestPrefetchRatio = 0.0f;
+
+ for (int region = 0; region < cluster.numRegions; region++) {
+ prefetchRatio +=
+ cluster.getOrComputeWeightedPrefetchRatio(region,
cluster.regionIndexToServerIndex[region]);
+ bestPrefetchRatio += cluster.getOrComputeWeightedPrefetchRatio(region,
+ cluster.getOrComputeServerWithBestPrefetchRatio()[region]);
+ }
+ prefetchRatio = bestPrefetchRatio == 0.0f ? 1.0f : prefetchRatio /
bestPrefetchRatio;
+ }
+
+ @Override
+ protected double cost() {
+ return 1 - prefetchRatio;
+ }
+
+ @Override
+ protected void regionMoved(int region, int oldServer, int newServer) {
+ float oldServerPrefetch =
cluster.getOrComputeWeightedPrefetchRatio(region, oldServer);
+ float newServerPrefetch =
cluster.getOrComputeWeightedPrefetchRatio(region, newServer);
+ float prefetchDelta = newServerPrefetch - oldServerPrefetch;
+ float normalizeDelta = bestPrefetchRatio == 0.0f ? 0.0f : prefetchDelta /
bestPrefetchRatio;
+ prefetchRatio += normalizeDelta;
+ }
Review Comment:
This is also difficult to follow. Shouldn't this just repeat same
calculations from `prepare()`, but using the newServerPrefetch in the place of
` cluster.getOrComputeWeightedPrefetchRatio(region,
cluster.regionIndexToServerIndex[region]);` ?
--
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]
