krconv commented on code in PR #6623:
URL: https://github.com/apache/hbase/pull/6623#discussion_r1924453449
##########
hbase-protocol-shaded/src/main/protobuf/client/Client.proto:
##########
@@ -274,6 +278,7 @@ message Scan {
}
optional ReadType readType = 23 [default = DEFAULT];
optional bool need_cursor_result = 24 [default = false];
+ optional bool resultMetricsEnabled = 25 [default = false];
Review Comment:
Nit; `enable_result_metrics` matches the existing fields better (and same
note for other protobuf changes)
```suggestion
optional bool enable_result_metrics = 25 [default = false];
```
##########
hbase-client/src/main/java/org/apache/hadoop/hbase/client/Get.java:
##########
@@ -95,6 +95,7 @@ public Get(Get get) {
this.setFilter(get.getFilter());
this.setReplicaId(get.getReplicaId());
this.setConsistency(get.getConsistency());
+ this.setResultMetricsEnabled(get.isResultMetricsEnabled());
Review Comment:
Nit; Thoughts on the name `QueryMetrics`? I think that `ResultMetrics` could
be interpreted as "metrics about the result" instead of "metrics about the
query operation"
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RSRpcServices.java:
##########
@@ -3425,6 +3438,12 @@ private void scan(HBaseRpcController controller,
ScanRequest request, RegionScan
}
boolean mayHaveMoreCellsInRow =
scannerContext.mayHaveMoreCellsInRow();
Result r = Result.create(values, null, stale,
mayHaveMoreCellsInRow);
+
+ if (request.getScan().getResultMetricsEnabled()) {
Review Comment:
What's the reason for not directly setting it on the result for scans?
##########
hbase-protocol-shaded/src/main/protobuf/client/Client.proto:
##########
@@ -458,6 +466,13 @@ message RegionAction {
optional Condition condition = 4;
}
+/*
+* Statistics about the Result's server-side metrics
+*/
+message ResultMetrics {
+ required uint64 blockBytesScanned = 1;
Review Comment:
Not sure what the standard is for HBase, but I know that [optional values
with a default are more future proof than required
fields](https://stackoverflow.com/a/31814967), and I think that would be better
##########
hbase-client/src/main/java/org/apache/hadoop/hbase/shaded/protobuf/ProtobufUtil.java:
##########
@@ -1434,6 +1439,10 @@ public static ClientProtos.Result toResult(final Result
result, boolean encodeTa
builder.setStale(result.isStale());
builder.setPartial(result.mayHaveMoreCellsInRow());
+ if (result.getMetrics() != null) {
Review Comment:
Seems like the [result metrics will get lost for `exists`
calls](https://github.com/apache/hbase/pull/6623/files#diff-32e244a21a1e538afa88588f8ced6ff24a107fd5aac01038c528d3d27ac844dbR1454-R1460);
thoughts on handling that more explicitly? Maybe throwing an error if someone
tries to enable metrics on an Get with `checkExistenceOnly == true`
--
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]
