virajjasani commented on code in PR #6868:
URL: https://github.com/apache/hbase/pull/6868#discussion_r2056718063
##########
hbase-client/src/main/java/org/apache/hadoop/hbase/client/Scan.java:
##########
@@ -115,6 +115,8 @@ public class Scan extends Query {
// define this attribute with the appropriate table name by calling
// scan.setAttribute(Scan.SCAN_ATTRIBUTES_TABLE_NAME,
Bytes.toBytes(tableName))
static public final String SCAN_ATTRIBUTES_TABLE_NAME =
"scan.attributes.table.name";
+ static private final String SCAN_ATTRIBUTES_METRICS_BY_REGION_ENABLE =
+ "scan.attributes.metrics.byRegion.enable";
Review Comment:
nit: `scan.attributes.metrics.byregion.enable`
##########
hbase-client/src/main/java/org/apache/hadoop/hbase/client/metrics/ServerSideScanMetrics.java:
##########
@@ -117,4 +121,64 @@ public Map<String, Long> getMetricsMap(boolean reset) {
// Build the immutable map so that people can't mess around with it.
return builder.build();
}
+
+ public ServerName getServerName() {
+ return this.serverName;
+ }
+
+ public void setServerName(ServerName serverName) {
+ if (this.serverName == null) {
+ this.serverName = serverName;
+ }
+ }
+
+ public void setEncodedRegionName(String encodedRegionName) {
+ if (this.encodedRegionName == null) {
+ this.encodedRegionName = encodedRegionName;
+ }
+ }
+
+ public String getEncodedRegionName() {
+ return this.encodedRegionName;
+ }
+
+ @Override
+ public String toString() {
+ StringBuilder sb = new StringBuilder();
+ sb.append("ServerName:");
+ sb.append(this.serverName);
+ sb.append(",EncodedRegion:");
+ sb.append(this.encodedRegionName);
+ sb.append(",[");
+ boolean isFirstMetric = true;
+ for (Map.Entry<String, AtomicLong> e : this.counters.entrySet()) {
+ if (isFirstMetric) {
+ isFirstMetric = false;
+ } else {
+ sb.append(",");
+ }
+ sb.append(e.getKey());
+ sb.append(":");
+ sb.append(e.getValue().get());
+ }
+ sb.append("]");
+ return sb.toString();
+ }
Review Comment:
nit: you might want to consider using
`org.apache.commons.lang3.builder.ToStringBuilder`, it takes care of null and
other formatting concerns.
e.g.
```
@Override
public String toString() {
return new ToStringBuilder(this)
.append("counters", counters)
.append("serverName", serverName)
.append("encodedRegionName", encodedRegionName)
.append("countOfRowsFiltered", countOfRowsFiltered)
.append("countOfRowsScanned", countOfRowsScanned)
.append("countOfBlockBytesScanned", countOfBlockBytesScanned)
.append("fsReadTime", fsReadTime)
.toString();
}
```
##########
hbase-client/src/main/java/org/apache/hadoop/hbase/client/AbstractClientScanner.java:
##########
@@ -44,6 +64,24 @@ protected void initScanMetrics(Scan scan) {
*/
@Override
public ScanMetrics getScanMetrics() {
- return scanMetrics;
+ if (scanMetricsByRegion != null) {
+ if (scanMetricsByRegion.isEmpty()) {
+ return null;
Review Comment:
Don't we want to return empty metrics rather than null?
##########
hbase-client/src/main/java/org/apache/hadoop/hbase/client/Scan.java:
##########
@@ -1030,4 +1032,15 @@ public boolean isNeedCursorResult() {
public static Scan createScanFromCursor(Cursor cursor) {
return new Scan().withStartRow(cursor.getRow());
}
+
+ public Scan setEnableScanMetricsByRegion(final boolean enable) {
+ setAttribute(Scan.SCAN_ATTRIBUTES_METRICS_BY_REGION_ENABLE,
+ Bytes.toBytes(Boolean.valueOf(enable)));
Review Comment:
nit: `Bytes.toBytes(enable)`
##########
hbase-client/src/main/java/org/apache/hadoop/hbase/client/metrics/ServerSideScanMetrics.java:
##########
@@ -117,4 +121,64 @@ public Map<String, Long> getMetricsMap(boolean reset) {
// Build the immutable map so that people can't mess around with it.
return builder.build();
}
+
+ public ServerName getServerName() {
+ return this.serverName;
+ }
+
+ public void setServerName(ServerName serverName) {
+ if (this.serverName == null) {
+ this.serverName = serverName;
+ }
+ }
+
+ public void setEncodedRegionName(String encodedRegionName) {
+ if (this.encodedRegionName == null) {
+ this.encodedRegionName = encodedRegionName;
+ }
+ }
+
+ public String getEncodedRegionName() {
+ return this.encodedRegionName;
+ }
+
+ @Override
+ public String toString() {
+ StringBuilder sb = new StringBuilder();
+ sb.append("ServerName:");
+ sb.append(this.serverName);
+ sb.append(",EncodedRegion:");
+ sb.append(this.encodedRegionName);
+ sb.append(",[");
+ boolean isFirstMetric = true;
+ for (Map.Entry<String, AtomicLong> e : this.counters.entrySet()) {
+ if (isFirstMetric) {
+ isFirstMetric = false;
+ } else {
+ sb.append(",");
+ }
+ sb.append(e.getKey());
+ sb.append(":");
+ sb.append(e.getValue().get());
+ }
+ sb.append("]");
+ return sb.toString();
+ }
+
+ public void combineMetrics(ScanMetrics other) {
+ for (Map.Entry<String, AtomicLong> entry : other.counters.entrySet()) {
+ String counterName = entry.getKey();
+ AtomicLong counter = entry.getValue();
+ this.addToCounter(counterName, counter.get());
+ }
+ if (
+ this.encodedRegionName != null
+ && !Objects.equals(this.encodedRegionName,
other.getEncodedRegionName())
+ ) {
+ this.encodedRegionName = null;
+ }
+ if (this.serverName != null && !Objects.equals(this.serverName,
other.getServerName())) {
+ this.serverName = null;
Review Comment:
Curious, how does setting null for `encodedRegionName` and `serverName`
help? Are we using them to log something or return them with ScanMetrics?
--
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]
