haridsv commented on code in PR #2337: URL: https://github.com/apache/phoenix/pull/2337#discussion_r2656161501
########## phoenix-core-client/src/main/java/org/apache/phoenix/monitoring/SlowestScanMetricsQueue.java: ########## @@ -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.phoenix.monitoring; + +import java.util.Collections; +import java.util.Deque; +import java.util.Iterator; +import java.util.concurrent.ConcurrentLinkedDeque; + +/** + * Maintains a queue of {@link ScanMetricsHolder} instances, one instance per HBase scan created by + * Phoenix client. The class exposes an iterator which returns ScanMetricsHolder instances in + * reverse order of insertion as the last inserted instance is more likely to correspond to the + * slowest HBase scan. + * <p> + * The insertion of ScanMetricsHolder instances to the queue is thread-safe, so multiple parallel + * scans can concurrently add their ScanMetricsHolder instances to the queue. + */ +public class SlowestScanMetricsQueue { + + /** + * A no-op implementation that ignores all additions and returns an empty iterator. + */ + public static final SlowestScanMetricsQueue NOOP_SLOWEST_SCAN_METRICS_QUEUE = + new SlowestScanMetricsQueue() { + @Override + public void add(ScanMetricsHolder scanMetricsHolder) { + } + + @Override + public Iterator<ScanMetricsHolder> getIterator() { + return Collections.emptyIterator(); + } + }; Review 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. ########## phoenix-core-client/src/main/java/org/apache/phoenix/monitoring/GlobalClientMetrics.java: ########## @@ -271,4 +273,36 @@ public static boolean isMetricsEnabled() { return isGlobalMetricsEnabled; } + private static void changeMetric(GlobalClientMetrics metric, Long value) { + if (value != null) { + metric.update(value); + } + } + + public static void populateMetrics(Map<String, Long> scanMetricsMap, Long dummyRowCounter) { + changeMetric(GLOBAL_SCAN_BYTES, scanMetricsMap.get(ScanMetrics.BYTES_IN_RESULTS_METRIC_NAME)); + changeMetric(GLOBAL_HBASE_COUNT_RPC_CALLS, + scanMetricsMap.get(ScanMetrics.RPC_CALLS_METRIC_NAME)); + changeMetric(GLOBAL_HBASE_COUNT_REMOTE_RPC_CALLS, + scanMetricsMap.get(ScanMetrics.REMOTE_RPC_CALLS_METRIC_NAME)); + changeMetric(GLOBAL_HBASE_COUNT_MILLS_BETWEEN_NEXTS, + scanMetricsMap.get(ScanMetrics.MILLIS_BETWEEN_NEXTS_METRIC_NAME)); + changeMetric(GLOBAL_HBASE_COUNT_NOT_SERVING_REGION_EXCEPTION, + scanMetricsMap.get(ScanMetrics.NOT_SERVING_REGION_EXCEPTION_METRIC_NAME)); + changeMetric(GLOBAL_HBASE_COUNT_BYTES_REGION_SERVER_RESULTS, + scanMetricsMap.get(ScanMetrics.BYTES_IN_RESULTS_METRIC_NAME)); + changeMetric(GLOBAL_HBASE_COUNT_BYTES_IN_REMOTE_RESULTS, + scanMetricsMap.get(ScanMetrics.BYTES_IN_REMOTE_RESULTS_METRIC_NAME)); + changeMetric(GLOBAL_HBASE_COUNT_SCANNED_REGIONS, + scanMetricsMap.get(ScanMetrics.REGIONS_SCANNED_METRIC_NAME)); + changeMetric(GLOBAL_HBASE_COUNT_RPC_RETRIES, + scanMetricsMap.get(ScanMetrics.RPC_RETRIES_METRIC_NAME)); + changeMetric(GLOBAL_HBASE_COUNT_REMOTE_RPC_RETRIES, + scanMetricsMap.get(ScanMetrics.REMOTE_RPC_RETRIES_METRIC_NAME)); + changeMetric(GLOBAL_HBASE_COUNT_ROWS_SCANNED, + 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); Review Comment: Shouldn't this code driven by a list? ########## phoenix-core-client/src/main/java/org/apache/phoenix/monitoring/ScanMetricsHolder.java: ########## @@ -194,6 +212,94 @@ public void setScanMetricMap(Map<String, Long> scanMetricMap) { this.scanMetricMap = scanMetricMap; } + public void + setScanMetricsByRegion(Map<ScanMetricsRegionInfo, Map<String, Long>> scanMetricsByRegion) { + this.scanMetricsByRegion = scanMetricsByRegion; + } + + public void populateMetrics(Long dummyRowCounter) { + changeMetric(countOfRPCcalls, scanMetricMap.get(ScanMetrics.RPC_CALLS_METRIC_NAME)); + changeMetric(countOfRemoteRPCcalls, + scanMetricMap.get(ScanMetrics.REMOTE_RPC_CALLS_METRIC_NAME)); + changeMetric(sumOfMillisSecBetweenNexts, + scanMetricMap.get(ScanMetrics.MILLIS_BETWEEN_NEXTS_METRIC_NAME)); + changeMetric(countOfNSRE, + scanMetricMap.get(ScanMetrics.NOT_SERVING_REGION_EXCEPTION_METRIC_NAME)); + changeMetric(countOfBytesInResults, + scanMetricMap.get(ScanMetrics.BYTES_IN_RESULTS_METRIC_NAME)); + changeMetric(countOfBytesInRemoteResults, + scanMetricMap.get(ScanMetrics.BYTES_IN_REMOTE_RESULTS_METRIC_NAME)); + changeMetric(countOfRegions, scanMetricMap.get(ScanMetrics.REGIONS_SCANNED_METRIC_NAME)); + changeMetric(countOfRPCRetries, scanMetricMap.get(ScanMetrics.RPC_RETRIES_METRIC_NAME)); + changeMetric(countOfRemoteRPCRetries, + scanMetricMap.get(ScanMetrics.REMOTE_RPC_RETRIES_METRIC_NAME)); + changeMetric(countOfRowsScanned, + scanMetricMap.get(ScanMetrics.COUNT_OF_ROWS_SCANNED_KEY_METRIC_NAME)); + changeMetric(countOfRowsFiltered, + scanMetricMap.get(ScanMetrics.COUNT_OF_ROWS_FILTERED_KEY_METRIC_NAME)); + changeMetric(countOfBytesScanned, scanMetricMap.get(ScanMetrics.BYTES_IN_RESULTS_METRIC_NAME)); + changeMetric(countOfRowsPaged, dummyRowCounter); + changeMetric(fsReadTime, CompatScanMetrics.getFsReadTime(scanMetricMap)); + changeMetric(countOfBytesReadFromFS, CompatScanMetrics.getBytesReadFromFs(scanMetricMap)); + changeMetric(countOfBytesReadFromMemstore, + CompatScanMetrics.getBytesReadFromMemstore(scanMetricMap)); + changeMetric(countOfBytesReadFromBlockcache, + CompatScanMetrics.getBytesReadFromBlockCache(scanMetricMap)); + changeMetric(countOfBlockReadOps, CompatScanMetrics.getBlockReadOpsCount(scanMetricMap)); + changeMetric(rpcScanProcessingTime, CompatScanMetrics.getRpcScanProcessingTime(scanMetricMap)); + changeMetric(rpcScanQueueWaitTime, CompatScanMetrics.getRpcScanQueueWaitTime(scanMetricMap)); Review 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); } ``` ########## phoenix-core-client/src/main/java/org/apache/phoenix/util/JDBCUtil.java: ########## @@ -148,6 +148,26 @@ public static int getMutateBatchSize(String url, Properties info, ReadOnlyProps : Integer.parseInt(batchSizeStr)); } + public static int getSlowestScanMetricsCount(String url, Properties info, ReadOnlyProps props) + throws SQLException { + String slowestScanMetricsCountStr = + findProperty(url, info, QueryServices.SLOWEST_SCAN_METRICS_COUNT); + return slowestScanMetricsCountStr == null + ? props.getInt(QueryServices.SLOWEST_SCAN_METRICS_COUNT, + QueryServicesOptions.DEFAULT_SLOWEST_SCAN_METRICS_COUNT) + : Integer.parseInt(slowestScanMetricsCountStr); + } Review Comment: Nit: Why not extract a generic method like `getIntFromConnectionInfo()` from the existing `getMutateBatchSize` and reuse it here? ########## phoenix-core-client/src/main/java/org/apache/phoenix/jdbc/ParallelPhoenixResultSet.java: ########## @@ -133,6 +136,17 @@ public Map<String, Map<MetricType, Long>> getReadMetrics() { return metrics; } + @Override + public List<List<JsonObject>> getTopNSlowestScanMetrics() { + List<List<JsonObject>> metrics; + if (rs != null) { + metrics = ((PhoenixMonitoredResultSet) rs).getTopNSlowestScanMetrics(); + } else { + metrics = Collections.emptyList(); + } + return metrics; + } + Review Comment: Perhaps this repeating method can be moved into JDBCUtil? -- 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]
