yashmayya commented on code in PR #16007:
URL: https://github.com/apache/pinot/pull/16007#discussion_r2149106547
##########
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);
Review Comment:
What's the use case for allowing per-query configurability of this stats
logging level?
##########
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()
Review Comment:
nit: could we call it something like `completionStatus` instead? Seems a
little strange to have a string called `successfullyStr` that could contain
`with errors ...`
##########
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:
I don't understand this condition. If the broker response has exceptions why
are we checking whether `successfulSummarizeLevel` is `INFO` / `WARN` / `ERROR`
to decide whether to use the configured logging level?
##########
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
Review Comment:
Let's add a log line here stating that the specified log level was invalid
and we're defaulting to `DEBUG`?
--
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]
