You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by ja...@apache.org on 2020/06/10 22:11:10 UTC
[incubator-pinot] branch master updated: Clean up the usage of
BrokerRequest in metrics (#5535)
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 <17...@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-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org