HBASE-29233: Capture scan metrics at region level #6868
There are no files selected for viewing
This file contains hidden or 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,3 +25,4 @@ linklint/ | |
| **/*.log | ||
| tmp | ||
| **/.flattened-pom.xml | ||
| .vscode/ | ||
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,77 @@ | ||
| /* | ||
| * 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.client.metrics; | ||
|
|
||
| import java.util.HashMap; | ||
| import java.util.Map; | ||
| import java.util.concurrent.atomic.AtomicLong; | ||
| import org.apache.hadoop.hbase.ServerName; | ||
| import org.apache.yetus.audience.InterfaceAudience; | ||
|
|
||
| /** | ||
| * Captures region level scan metrics as a map of metric name ({@link String}) -> Value | ||
| * ({@link AtomicLong}). <br/> | ||
| * <br/> | ||
| * One instance stores scan metrics for a single region only. | ||
| */ | ||
| @InterfaceAudience.Private | ||
| public class RegionScanMetricsData { | ||
| private final Map<String, AtomicLong> counters = new HashMap<>(); | ||
| private ScanMetricsRegionInfo scanMetricsRegionInfo = | ||
| ScanMetricsRegionInfo.EMPTY_SCAN_METRICS_REGION_INFO; | ||
|
|
||
| AtomicLong createCounter(String counterName) { | ||
| return ScanMetricsUtil.createCounter(counters, counterName); | ||
| } | ||
|
|
||
| void setCounter(String counterName, long value) { | ||
| ScanMetricsUtil.setCounter(counters, counterName, value); | ||
| } | ||
|
|
||
| void addToCounter(String counterName, long delta) { | ||
| ScanMetricsUtil.addToCounter(counters, counterName, delta); | ||
| } | ||
|
|
||
| Map<String, Long> collectMetrics(boolean reset) { | ||
| return ScanMetricsUtil.collectMetrics(counters, reset); | ||
| } | ||
|
|
||
| @Override | ||
| public String toString() { | ||
| return getClass().getSimpleName() + "[" + scanMetricsRegionInfo + "," + "Counters=" + counters | ||
| + "]"; | ||
| } | ||
|
|
||
| /** | ||
| * Populate encoded region name and server name details if not already populated. If details are | ||
| * already populated and a re-attempt is done then {@link UnsupportedOperationException} is | ||
| * thrown. | ||
| */ | ||
| void initScanMetricsRegionInfo(String encodedRegionName, ServerName serverName) { | ||
| // Check by reference | ||
| if (scanMetricsRegionInfo == ScanMetricsRegionInfo.EMPTY_SCAN_METRICS_REGION_INFO) { | ||
| scanMetricsRegionInfo = new ScanMetricsRegionInfo(encodedRegionName, serverName); | ||
| } else { | ||
| throw new UnsupportedOperationException("ScanMetricsRegionInfo has already been initialized"); | ||
| } | ||
| } | ||
|
|
||
| ScanMetricsRegionInfo getScanMetricsRegionInfo() { | ||
| return scanMetricsRegionInfo; | ||
| } | ||
| } |
This file contains hidden or 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
This file contains hidden or 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,81 @@ | ||
| /* | ||
| * 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.client.metrics; | ||
|
|
||
| import org.apache.commons.lang3.builder.EqualsBuilder; | ||
| import org.apache.commons.lang3.builder.HashCodeBuilder; | ||
| import org.apache.hadoop.hbase.ServerName; | ||
| import org.apache.yetus.audience.InterfaceAudience; | ||
| import org.apache.yetus.audience.InterfaceStability; | ||
|
|
||
| /** | ||
| * POJO for capturing region level details when region level scan metrics are enabled. <br> | ||
| * <br> | ||
| * Currently, encoded region name and server name (host name, ports and startcode) are captured as | ||
| * region details. <br> | ||
| * <br> | ||
| * Instance of this class serves as key in the Map returned by | ||
| * {@link ServerSideScanMetrics#collectMetricsByRegion()} or | ||
| * {@link ServerSideScanMetrics#collectMetricsByRegion(boolean)}. | ||
| */ | ||
| @InterfaceAudience.Public | ||
| @InterfaceStability.Evolving | ||
| public class ScanMetricsRegionInfo { | ||
|
Contributor
There was a problem hiding this comment. While we are not expecting much changes, until we have gained more confidence after prod use, let's also add |
||
| /** | ||
| * Users should only compare against this constant by reference and should not make any | ||
| * assumptions regarding content of the constant. | ||
| */ | ||
| public static final ScanMetricsRegionInfo EMPTY_SCAN_METRICS_REGION_INFO = | ||
| new ScanMetricsRegionInfo(null, null); | ||
|
|
||
| private final String encodedRegionName; | ||
| private final ServerName serverName; | ||
|
|
||
| ScanMetricsRegionInfo(String encodedRegionName, ServerName serverName) { | ||
|
Contributor
There was a problem hiding this comment. Why non-public constructor in a public class?
Contributor
Author
There was a problem hiding this comment. This is intentional as I don't want users to instantiate an instance of this class by themselves from outside HBase code. The public facing part of this class is the getters but not constructor. |
||
| this.encodedRegionName = encodedRegionName; | ||
| this.serverName = serverName; | ||
| } | ||
|
|
||
| @Override | ||
| public boolean equals(Object obj) { | ||
|
Member
There was a problem hiding this comment. Commons lang has Equals and HashCode Builders that you can use out of the box. |
||
| if (!(obj instanceof ScanMetricsRegionInfo other)) { | ||
| return false; | ||
| } | ||
| return new EqualsBuilder().append(encodedRegionName, other.encodedRegionName) | ||
| .append(serverName, other.serverName).isEquals(); | ||
| } | ||
|
|
||
| @Override | ||
| public int hashCode() { | ||
| return new HashCodeBuilder(17, 37).append(encodedRegionName).append(serverName).toHashCode(); | ||
| } | ||
|
|
||
| @Override | ||
| public String toString() { | ||
| return getClass().getSimpleName() + "[encodedRegionName=" + encodedRegionName + ",serverName=" | ||
| + serverName + "]"; | ||
| } | ||
|
|
||
| public String getEncodedRegionName() { | ||
| return encodedRegionName; | ||
| } | ||
|
|
||
| public ServerName getServerName() { | ||
| return serverName; | ||
| } | ||
| } | ||
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should make it so that if user enables scanMetricsByRegion, then scanMetrics also gets initialized (even if scanMetricsEnabled is not true).
On the other hand, the current approach is also good but now we need user to set both scanMetrics and scanMetricsByRegion to true, two flags for one feature?
I don't have strong opinion here btw, i was just thinking about this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I also thought the same but I also didn't have any strong opinion so, kept it this way as was wondering what to do or should I handle the case when
isScanMetricsEnabled()isfalseandisScanMetricsByRegionEnabled()is true given I will be setting scanMetricsEnabled if scanMetricsByRegion are enabled. So, kept it simple.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe single flag per feature would make it simpler from user's viewpoint:
scanMetricsto enable scan metricsscanMetricsByRegionto enable per region scan metricsLet's also wait for Nick's suggestion here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it makes sense to have a flag per feature (you have to retain the old flag for backwards compatibility). I think it also makes sense that that
isScanMetricsByRegionEnabled=trueimpliesisScanMetricsEnabled=true-- enabling the new feature implicitly enables the old feature.How you capture and accurately propagate this logic through the
Scanstate management will be an (perhaps annoying) implementation detail.