PHOENIX-7729: Identify top N slowest parallel scans for metrics reporting #2337
Conversation
… memstore and block count read from FS
| <activation> | ||
| <property> | ||
| <name>!hbase.profile</name> | ||
| <name>hbase.profile</name> |
There was a problem hiding this comment.
Revert all the changes in this file before merge.
| scan.setScanMetricsEnabled(true); | ||
| scan.setEnableScanMetricsByRegion(isScanMetricsByRegionEnabled); |
There was a problem hiding this comment.
These 2 calls as a side effect of the getter method is not a good pattern.
| globalScanMetrics.get(ScanMetrics.MILLIS_BETWEEN_NEXTS_METRIC_NAME); | ||
| } | ||
|
|
||
| private MetricType getMetricType(String metricName) { |
There was a problem hiding this comment.
How about moving this to MetricType enum? You could even add the HBase metric name to the enum constructor and create a lookup table dynamically by introspecting the enums with non-null HBase metric name. You can even have static getters to access the metric names and drive other parts of the code that have a long list of getters and setters to execute the logic reflectively instead of statically. A getter on the enum to access the hbase metric name can also help access the names via the enum for consistency.
| public Iterator<ScanMetricsHolder> getIterator() { | ||
| return Collections.emptyIterator(); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Nit: This still creates an empty ConcurrentLinkedDeque that never gets used. You can add an alternative (private) constructor that takes a list and you can pass null for it.
| CompatScanMetrics.getBytesReadFromBlockCache(scanMetricMap)); | ||
| changeMetric(countOfBlockReadOps, CompatScanMetrics.getBlockReadOpsCount(scanMetricMap)); | ||
| changeMetric(rpcScanProcessingTime, CompatScanMetrics.getRpcScanProcessingTime(scanMetricMap)); | ||
| changeMetric(rpcScanQueueWaitTime, CompatScanMetrics.getRpcScanQueueWaitTime(scanMetricMap)); |
There was a problem hiding this comment.
Why not have a `Map<MetricType, CombinableMetric> metrics field?
If you add something like List<MetricType> getHBaseBasedMetricTypes() to MetricType, then you can replace all but SCAN_BYTES and PAGED_ROWS_COUNTER with the below code:
// Allocate all HBase-backed metrics in a loop
for (MetricType metricType : MetricTypes.getHBaseBasedMetricTypes()) {
metrics.put(metricType, readMetrics.allotMetric(metricType, tableName));
}
As for the getters and setters, we can have one generic method like:
public CombinableMetric getMetric(MetricType type) {
return metrics.get(type);
}
or if we prefer specific typed names, they can be changed like this:
public CombinableMetric getCountOfRemoteRPCcalls() {
return metrics.get(COUNT_REMOTE_RPC_CALLS);
}
| ? props.getInt(QueryServices.SLOWEST_SCAN_METRICS_COUNT, | ||
| QueryServicesOptions.DEFAULT_SLOWEST_SCAN_METRICS_COUNT) | ||
| : Integer.parseInt(slowestScanMetricsCountStr); | ||
| } |
There was a problem hiding this comment.
Nit: Why not extract a generic method like getIntFromConnectionInfo() from the existing getMutateBatchSize and reuse it here?
| } | ||
| return metrics; | ||
| } | ||
|
|
There was a problem hiding this comment.
Perhaps this repeating method can be moved into JDBCUtil?
| scanMetricsMap.get(ScanMetrics.COUNT_OF_ROWS_SCANNED_KEY_METRIC_NAME)); | ||
| changeMetric(GLOBAL_HBASE_COUNT_ROWS_FILTERED, | ||
| scanMetricsMap.get(ScanMetrics.COUNT_OF_ROWS_FILTERED_KEY_METRIC_NAME)); | ||
| changeMetric(GLOBAL_PAGED_ROWS_COUNTER, dummyRowCounter); |
There was a problem hiding this comment.
Shouldn't this code driven by a list?
2 participants
JIRA: PHOENIX-7729
phoenix.slowest.scan.metrics.countandphoenix.scan.metrics.by.region.enabled.phoenix.slowest.scan.metrics.countspecifies the value of N for capturing metrics for top N slowest scan metrics. The value should be > 0 to enable capturing top N slowest scan metrics.phoenix.scan.metrics.by.region.enabledspecifies whether to capture region hash and RS name along with HBase scan metrics. Leverages HBASE-29233.SlowestScanMetricsIT.