snleee commented on a change in pull request #7260:
URL: https://github.com/apache/pinot/pull/7260#discussion_r683975450
##########
File path:
pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java
##########
@@ -190,6 +190,9 @@
public static final double DEFAULT_BROKER_QUERY_LOG_MAX_RATE_PER_SECOND =
10_000d;
public static final String CONFIG_OF_BROKER_TIMEOUT_MS =
"pinot.broker.timeoutMs";
public static final long DEFAULT_BROKER_TIMEOUT_MS = 10_000L;
+ public static final String
CONFIG_OF_BROKER_SLOW_QUERY_LATENCY_THRESHOLD_MS =
+ "pinot.broker.slow.query.latency.thresholdMs";
+ public static final long DEFAULT_BROKER_SLOW_QUERY_LATENCY_THRESHOLD_MS =
500L;
Review comment:
I would like to be conservative for the default number (may be 1second).
Otherwise, this may suddenly start to add a lot of logs for some use cases
whose queries are usually take > 500ms.
##########
File path:
pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java
##########
@@ -244,6 +247,9 @@
public static final String CONFIG_OF_QUERY_EXECUTOR_TIMEOUT =
"pinot.server.query.executor.timeout";
public static final String CONFIG_OF_QUERY_EXECUTOR_CLASS =
"pinot.server.query.executor.class";
public static final String CONFIG_OF_REQUEST_HANDLER_FACTORY_CLASS =
"pinot.server.requestHandlerFactory.class";
+ public static final String
CONFIG_OF_SERVER_SLOW_QUERY_LATENCY_THRESHOLD_MS =
+ "pinot.server.slow.query.latency.thresholdMs";
+ public static final long DEFAULT_SERVER_SLOW_QUERY_LATENCY_THRESHOLD_MS =
100L;
Review comment:
It looks that we are re-using the existing default threshold but it's
pretty surprising that we set this low number for logging slow query and not
have seen too much of the logs.
##########
File path:
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java
##########
@@ -532,6 +536,10 @@ private void logBrokerResponse(long requestId, String
query, RequestStatistics r
requestStatistics.getReduceTimeMillis(),
brokerResponse.getExceptionsSize(), serverStats.getServerStats(),
brokerResponse.getOfflineThreadCpuTimeNs(),
brokerResponse.getRealtimeThreadCpuTimeNs(),
StringUtils.substring(query, 0, _queryLogLength));
+ if (totalTimeMs > _brokerSlowQueryLatencyThreshold) {
+ LOGGER.warn("Slow query: requestId={}, table={}, timeMs={}", requestId,
Review comment:
Do we really need to put this extra line? We are basically logging the
same thing twice when the total time ms is larger than the threshold. In
practice, we can grep `timeMs > 500` to get the list of slow queries pretty
easily.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
