haridsv commented on code in PR #2329:
URL: https://github.com/apache/phoenix/pull/2329#discussion_r2614598538
##########
phoenix-core-server/src/main/java/org/apache/phoenix/coprocessor/UncoveredGlobalIndexRegionScanner.java:
##########
@@ -134,6 +143,13 @@ protected void scanDataRows(Collection<byte[]>
dataRowKeys, long startTime) thro
+ region.getRegionInfo().getRegionNameAsString() + " could not
complete on time (in "
+ pageSizeMs + " ms) and" + " will be resubmitted");
}
+ if (isScanMetricsEnabled) {
+ Map<String, Long> scanMetrics =
resultScanner.getScanMetrics().getMetricsMap();
Review Comment:
Instead of converting to Map, have you considered exposing the ScanMetrics
object directly? We could have the 2.6.4 compat class have 1:1 static final
constants for each of the relevant metric names. The older compat classes will
have those same constants set to dummy values. In Phoenix you would use
ScanMetrics.hasCounter() to consume them conditionally. This may not look
prettier than the current approach, but it will avoid creating additional
garbage via maps. In fact, you can also add have something like "public static
boolean supportsFineGrainedReadMetrics()" and you can have
`isScanMetricsEnabled` derived from both scan.isScanMetricsEnabled() and this
method which can avoid some unnecessary initialization on the older HBase
versions. E.g., in the above call, getMetricsMap() returns a valid map even in
the older HBase versions but we won't end up using any of the fields. Even the
list initialization can be avoided.
##########
phoenix-core-server/src/main/java/org/apache/phoenix/coprocessor/DataTableScanMetrics.java:
##########
@@ -0,0 +1,124 @@
+/*
+ * 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.coprocessor;
+
+import java.util.Map;
+import org.apache.hadoop.hbase.monitoring.ThreadLocalServerSideScanMetrics;
+import org.apache.phoenix.compat.hbase.CompatScanMetrics;
+import org.apache.phoenix.compat.hbase.CompatThreadLocalServerSideScanMetrics;
+
+/**
+ * Stores scan metrics from data table operations performed during:
+ * <ul>
+ * <li>Uncovered global index scans</li>
+ * <li>Read repair operations</li>
+ * </ul>
+ * These metrics help identify latency variations that occur when both data
table and index table
+ * are scanned together, and are used to populate {@link
ThreadLocalServerSideScanMetrics} for index
+ * table RPC calls.
+ */
+public class DataTableScanMetrics {
+ private final long fsReadTimeInMs;
+ private final long bytesReadFromFS;
+ private final long bytesReadFromMemstore;
+ private final long bytesReadFromBlockcache;
+ private final long blockReadOps;
+
+ protected DataTableScanMetrics(long fsReadTimeInMs, long bytesReadFromFS,
+ long bytesReadFromMemstore, long bytesReadFromBlockcache, long
blockReadOps) {
+ this.fsReadTimeInMs = fsReadTimeInMs;
+ this.bytesReadFromFS = bytesReadFromFS;
+ this.bytesReadFromMemstore = bytesReadFromMemstore;
+ this.bytesReadFromBlockcache = bytesReadFromBlockcache;
+ this.blockReadOps = blockReadOps;
+ }
+
+ public long getFsReadTimeInMs() {
+ return fsReadTimeInMs;
+ }
+
+ public long getBytesReadFromFS() {
+ return bytesReadFromFS;
+ }
+
+ public long getBytesReadFromMemstore() {
+ return bytesReadFromMemstore;
+ }
+
+ public long getBytesReadFromBlockcache() {
+ return bytesReadFromBlockcache;
+ }
+
+ public long getBlockReadOps() {
+ return blockReadOps;
+ }
+
+ public static class Builder {
+ protected long fsReadTimeInMs = 0;
+ protected long bytesReadFromFS = 0;
+ protected long bytesReadFromMemstore = 0;
+ protected long bytesReadFromBlockcache = 0;
+ protected long blockReadOps = 0;
+
+ public Builder setFsReadTimeInMs(long fsReadTimeInMs) {
+ this.fsReadTimeInMs = fsReadTimeInMs;
+ return this;
+ }
+
+ public Builder setBytesReadFromFS(long bytesReadFromFS) {
+ this.bytesReadFromFS = bytesReadFromFS;
+ return this;
+ }
+
+ public Builder setBytesReadFromMemstore(long bytesReadFromMemstore) {
+ this.bytesReadFromMemstore = bytesReadFromMemstore;
+ return this;
+ }
+
+ public Builder setBytesReadFromBlockcache(long bytesReadFromBlockcache) {
+ this.bytesReadFromBlockcache = bytesReadFromBlockcache;
+ return this;
+ }
+
+ public Builder setBlockReadOps(long blockReadOps) {
+ this.blockReadOps = blockReadOps;
+ return this;
+ }
+
+ public DataTableScanMetrics build() {
+ return new DataTableScanMetrics(fsReadTimeInMs, bytesReadFromFS,
bytesReadFromMemstore,
+ bytesReadFromBlockcache, blockReadOps);
+ }
+ }
+
+ public static void buildDataTableScanMetrics(Map<String, Long> scanMetrics,
Builder builder) {
Review Comment:
Per convention, this shouldn't be named buildXXX() because this doesn't
actually build the object of type XXX. Perhaps name it as populateXXX() or even
populateXXXBuilder()? Instead of populate, fill can also be a good choice.
Also, if this returns the `Builder`, then the calling code (e.g.,
[here](https://github.com/apache/phoenix/blob/209b8fb9da4df8d188b357dec9f18ae96cb44548/phoenix-core-server/src/main/java/org/apache/phoenix/coprocessor/UncoveredGlobalIndexRegionScanner.java#L252-L256)
and one more place) can maintain the builder call chain continuum.
--
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]
