gortiz commented on code in PR #16007:
URL: https://github.com/apache/pinot/pull/16007#discussion_r2149853296
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/MultiStageBrokerRequestHandler.java:
##########
@@ -200,6 +217,52 @@ protected BrokerResponse handleRequest(long requestId,
String query, SqlNodeAndO
}
}
+ private static Level successfulSummarizeLevel(SqlNodeAndOptions
sqlNodeAndOptions) {
+ String key =
CommonConstants.MultiStageQueryRunner.KEY_OF_SUCCESSFUL_SUMMARIZE_LOG;
+ String defaultValue =
CommonConstants.MultiStageQueryRunner.DEFAULT_OF_SUCCESSFUL_SUMMARIZE_LOG;
+ String str = sqlNodeAndOptions.getOptions().getOrDefault(key,
defaultValue);
+ try {
+ return Level.valueOf(StringUtils.upperCase(str));
+ } catch (IllegalArgumentException e) {
+ // If the value is not a valid Level, default to DEBUG
+ return Level.DEBUG;
+ }
+ }
+
+ private void summarizeQuery(BrokerResponse brokerResponse, Level
successfulSummarizeLevel) {
+ ObjectNode stats = brokerResponse instanceof BrokerResponseNativeV2
+ ? ((BrokerResponseNativeV2) brokerResponse).getStageStats()
+ : JsonNodeFactory.instance.objectNode();
+ String successfullyStr = brokerResponse.getExceptions().isEmpty()
+ ? "successfully"
+ : "with errors " + brokerResponse.getExceptions();
+ String logTemplate = "Request finished {} in {}ms. Stats: {}";
+ // We use the dynamic logging level in case there are no exceptions or the
successfulSummarizeLevel is
+ // INFO, WARN or ERROR (remember that ERROR < WARN < INFO).
+ if (brokerResponse.getExceptions().isEmpty() ||
successfulSummarizeLevel.compareTo(Level.INFO) <= 0) {
Review Comment:
Initially, I wanted to add this option as a way to conditionally log stats
on successful queries without spamming the log too much (although the
suggestion is to filter out using a log4j's BurstFilter)
But it sounds strange to log successful cases as WARN while erroneous cases
are logged as INFO, so I decided to bind them if the successful level was lower
than INFO (ERROR < WARN < INFO).
Something we can try is to modify the option and instead of using it as a
level, just use a boolean value, so successful cases are going to be logged as
INFO if the new query option is provided. This system is simpler to explain and
probably nobody was going to set the level property to something lower than INFO
--
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: commits-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
