This is an automated email from the ASF dual-hosted git repository. jackie pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/incubator-pinot.git
The following commit(s) were added to refs/heads/master by this push: new 7b41f5a Clean up the usage of BrokerRequest in metrics (#5535) 7b41f5a is described below commit 7b41f5ae7e544d39d06b938c0e91f746ebdf2771 Author: Xiaotian (Jackie) Jiang <17555551+jackie-ji...@users.noreply.github.com> AuthorDate: Wed Jun 10 15:11:01 2020 -0700 Clean up the usage of BrokerRequest in metrics (#5535) Pinot supports global metrics and table level metrics. This PR replaces `addMeteredQueryValue` with `addMeteredTableValue`. There is no functionality change. --- .../pinot/common/metrics/AbstractMetrics.java | 74 ---------------------- .../query/executor/ServerQueryExecutorV1Impl.java | 6 +- .../core/query/reduce/GroupByDataTableReducer.java | 5 +- 3 files changed, 6 insertions(+), 79 deletions(-) diff --git a/pinot-common/src/main/java/org/apache/pinot/common/metrics/AbstractMetrics.java b/pinot-common/src/main/java/org/apache/pinot/common/metrics/AbstractMetrics.java index 018928c..5178fc1 100644 --- a/pinot-common/src/main/java/org/apache/pinot/common/metrics/AbstractMetrics.java +++ b/pinot-common/src/main/java/org/apache/pinot/common/metrics/AbstractMetrics.java @@ -26,9 +26,7 @@ import java.util.concurrent.Callable; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicLong; -import javax.annotation.Nullable; import org.apache.pinot.common.Utils; -import org.apache.pinot.common.request.BrokerRequest; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -92,23 +90,6 @@ public abstract class AbstractMetrics<QP extends AbstractMetrics.QueryPhase, M e boolean isGlobal(); } - /** - * Logs the timing of a query phase. - * - * @param request The broker request associated with this query - * @param phase The query phase for which to log time - * @param duration The duration that the phase execution took to complete - * @param timeUnit The time unit of the duration - */ - public void addPhaseTiming(BrokerRequest request, QP phase, long duration, TimeUnit timeUnit) { - String fullTimerName = buildMetricName(request, phase.getQueryPhaseName()); - addValueToTimer(fullTimerName, duration, timeUnit); - } - - public void addPhaseTiming(BrokerRequest request, QP phase, long nanos) { - addPhaseTiming(request, phase, nanos, TimeUnit.NANOSECONDS); - } - public void addPhaseTiming(String tableName, QP phase, long duration, TimeUnit timeUnit) { String fullTimerName = _metricPrefix + getTableName(tableName) + "." + phase.getQueryPhaseName(); addValueToTimer(fullTimerName, duration, timeUnit); @@ -155,42 +136,6 @@ public abstract class AbstractMetrics<QP extends AbstractMetrics.QueryPhase, M e } /** - * Builds a complete metric name, of the form prefix.resource.metric - * - * @param request The broker request containing all the information - * @param metricName The metric name to register - * @return The complete metric name - */ - private String buildMetricName(@Nullable BrokerRequest request, String metricName) { - if (request != null && request.getQuerySource() != null && request.getQuerySource().getTableName() != null) { - return _metricPrefix + getTableName(request.getQuerySource().getTableName()) + "." + metricName; - } else { - return _metricPrefix + "unknown." + metricName; - } - } - - /** - * Logs the time taken to complete the given callable. - * - * @param request The broker request associated with this query - * @param phase The query phase - * @param callable The callable to execute - * @param <T> The return type of the callable - * @return The return value of the callable passed as a parameter - * @throws Exception The exception thrown by the callable - */ - public <T> T timeQueryPhase(final BrokerRequest request, final QP phase, final Callable<T> callable) - throws Exception { - long startTime = System.nanoTime(); - T returnValue = callable.call(); - long totalNanos = System.nanoTime() - startTime; - - addPhaseTiming(request, phase, totalNanos); - LOGGER.debug(" Phase: {} took {}ms", phase, TimeUnit.MILLISECONDS.convert(totalNanos, TimeUnit.NANOSECONDS)); - return returnValue; - } - - /** * Logs a value to a meter. * * @param meter The meter to use @@ -271,25 +216,6 @@ public abstract class AbstractMetrics<QP extends AbstractMetrics.QueryPhase, M e } /** - * Logs a value to a meter for a specific query. - * - * @param request The broker request associated with this query - * @param meter The meter to use - * @param unitCount The number of units to add to the meter - */ - public void addMeteredQueryValue(final BrokerRequest request, final M meter, final long unitCount) { - final String fullMeterName; - String meterName = meter.getMeterName(); - if (request != null) { - fullMeterName = buildMetricName(request, meterName); - } else { - fullMeterName = _metricPrefix + meterName; - } - final MetricName metricName = new MetricName(_clazz, fullMeterName); - MetricsHelper.newMeter(_metricsRegistry, metricName, meter.getUnit(), TimeUnit.SECONDS).mark(unitCount); - } - - /** * Logs a value to a table gauge. * * @param tableName The table name diff --git a/pinot-core/src/main/java/org/apache/pinot/core/query/executor/ServerQueryExecutorV1Impl.java b/pinot-core/src/main/java/org/apache/pinot/core/query/executor/ServerQueryExecutorV1Impl.java index db8dee1..aba102d 100644 --- a/pinot-core/src/main/java/org/apache/pinot/core/query/executor/ServerQueryExecutorV1Impl.java +++ b/pinot-core/src/main/java/org/apache/pinot/core/query/executor/ServerQueryExecutorV1Impl.java @@ -106,6 +106,7 @@ public class ServerQueryExecutorV1Impl implements QueryExecutor { long requestId = queryRequest.getRequestId(); BrokerRequest brokerRequest = queryRequest.getBrokerRequest(); + String tableNameWithType = queryRequest.getTableNameWithType(); LOGGER.debug("Incoming request Id: {}, query: {}", requestId, brokerRequest); // Use the timeout passed from the request if exists, or the instance-level timeout long queryTimeoutMs = _defaultTimeOutMs; @@ -120,7 +121,7 @@ public class ServerQueryExecutorV1Impl implements QueryExecutor { // Query scheduler wait time already exceeds query timeout, directly return if (remainingTimeMs <= 0) { - _serverMetrics.addMeteredQueryValue(brokerRequest, ServerMeter.SCHEDULING_TIMEOUT_EXCEPTIONS, 1); + _serverMetrics.addMeteredTableValue(tableNameWithType, ServerMeter.SCHEDULING_TIMEOUT_EXCEPTIONS, 1); String errorMessage = String .format("Query scheduling took %dms (longer than query timeout of %dms)", querySchedulingTimeMs, queryTimeoutMs); @@ -130,7 +131,6 @@ public class ServerQueryExecutorV1Impl implements QueryExecutor { return dataTable; } - String tableNameWithType = queryRequest.getTableNameWithType(); TableDataManager tableDataManager = _instanceDataManager.getTableDataManager(tableNameWithType); Preconditions.checkState(tableDataManager != null, "Failed to find data manager for table: " + tableNameWithType); @@ -224,7 +224,7 @@ public class ServerQueryExecutorV1Impl implements QueryExecutor { dataTable.getMetadata().put(DataTable.TOTAL_DOCS_METADATA_KEY, Long.toString(numTotalDocs)); } } catch (Exception e) { - _serverMetrics.addMeteredQueryValue(brokerRequest, ServerMeter.QUERY_EXECUTION_EXCEPTIONS, 1); + _serverMetrics.addMeteredTableValue(tableNameWithType, ServerMeter.QUERY_EXECUTION_EXCEPTIONS, 1); // Do not log error for BadQueryRequestException because it's caused by bad query if (e instanceof BadQueryRequestException) { diff --git a/pinot-core/src/main/java/org/apache/pinot/core/query/reduce/GroupByDataTableReducer.java b/pinot-core/src/main/java/org/apache/pinot/core/query/reduce/GroupByDataTableReducer.java index 5984026..a9fdd7c 100644 --- a/pinot-core/src/main/java/org/apache/pinot/core/query/reduce/GroupByDataTableReducer.java +++ b/pinot-core/src/main/java/org/apache/pinot/core/query/reduce/GroupByDataTableReducer.java @@ -164,7 +164,7 @@ public class GroupByDataTableReducer implements DataTableReducer { } if (brokerMetrics != null && resultSize > 0) { - brokerMetrics.addMeteredQueryValue(_brokerRequest, BrokerMeter.GROUP_BY_SIZE, resultSize); + brokerMetrics.addMeteredTableValue(tableName, BrokerMeter.GROUP_BY_SIZE, resultSize); } } @@ -193,7 +193,8 @@ public class GroupByDataTableReducer implements DataTableReducer { int index = _numGroupBy; int aggNum = 0; while (index < _numColumns) { - values[index] = AggregationFunctionUtils.getSerializableValue(_aggregationFunctions[aggNum++].extractFinalResult(values[index])); + values[index] = AggregationFunctionUtils + .getSerializableValue(_aggregationFunctions[aggNum++].extractFinalResult(values[index])); index++; } if (_sqlSelectionList != null) { --------------------------------------------------------------------- To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org