hgromer commented on PR #6623:
URL: https://github.com/apache/hbase/pull/6623#issuecomment-2685111709
> If the plan is to only include a `QueryMetrics` for `Result`s produced by
`Get`s, I think that you can reduce the scope of this class. Make it a
`GetMetrics` and have it be only applicable to `Get`-based queries. Push its
accessors down to the `Get` object, remove references from `Query` and `Scan`.
This approach might become weird because of how Gets are implemented as Scans
internally, or maybe it'll be fine -- that's essentially what you have already
done here.
I think it's nice to implement this behavior for all queries, which is what
I've done in this PR. I think it'd be weird to have
> You probably also need to update the object model of `ScanMetrics`,
because if a `Scan` is a `Query`, then a `ScanMetrics` should be a
`QueryMetrics`.
Just thinking this through a little bit, this makes sense b/c it enables us
to add metrics at a result level granularity specific to either scans or gets.
The only thing that gets tricky here is that you need to do class casting to
get the exact class you care about. So for example:
```java
class QueryMetrics {
public long getBlockBytesScanned();
}
class GetMetrics extends QueryMetrics {
public long getGetSpecificValue();
}
class Result {
public QueryMetrics getQueryMetrics();
}
```
Then, to access the metric
```java
class MyDriver() {
public static void main(String[] args) {
Get get = new Get(myRk).setQueryMetricsEnabled(true);
Result r = table.get(get);
GetMetrics = (GetMetrics) r.getQueryMetrics();
long getSpecificValue = r.getGetSpecificValue();
}
}
```
but maybe that's the price to pay for the added flexibility
> On the other hand, you can keep it as a `QueryMetrics` and also gather
this data for `Result` instances produced by a `Scan`. Then `Scan` users can
opt-into this metric so as to obtain fine-grained metrics over the internals of
the query.
> You'll also need think about whether enabling `QueryMetrics` also enables
`ScanMetrics` and vice-versa... I expect that there would be a configuration
state that enables only `ScanMetrics`, mimicking the behavior existing today.
This PR already opts for keeping the default behavior. The finer-grain query
metrics are only returned if `setQueryMetricsEnabled` is set to true. However,
that is distinct to `setScanMetricsEnabled`, which enables the existing scan
metrics. This allows for the user to enable each set of metrics independently
of on another, which I thought would be best.
> Whichever path you choose, you also need to wire this new metrics thing
into other parts of the system. Inspecting where `ScanMetrics` are used will
guide you -- MapReduce and PerformanceEvaluation tool(s) should also expose
this new feature.
Sounds good, will take a deeper look here
--
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]
