yashmayya commented on code in PR #16007:
URL: https://github.com/apache/pinot/pull/16007#discussion_r2149113589


##########
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:
   Oh, it's to ensure that the query option can't override the log level for 
errored query stats to ever be `DEBUG` or `TRACE`? IMO we should rename 
`successfulSummarizeLevel` because it's quite confusing to read here. I 
realized the intention after reading the description for the `summarizeLevel` 
query option in `CommonConstants`.



-- 
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

Reply via email to