Copilot commented on code in PR #17710:
URL: https://github.com/apache/pinot/pull/17710#discussion_r2812071709
##########
pinot-common/src/main/java/org/apache/pinot/common/metrics/ServerMeter.java:
##########
@@ -140,6 +140,10 @@ public enum ServerMeter implements AbstractMetrics.Meter {
RESPONSE_SER_MEM_ALLOCATED_BYTES("bytes", false),
TOTAL_MEM_ALLOCATED_BYTES("bytes", false),
LARGE_QUERY_RESPONSE_SIZE_EXCEPTIONS("exceptions", false),
+ /**
+ * Size of the serialized response sent from server to broker in bytes.
+ */
+ QUERY_RESPONSE_SIZE("bytes", false, "Size of the serialized response sent
from server to broker in bytes"),
Review Comment:
The `QUERY_RESPONSE_SIZE` description says the metric is for the response
"sent from server to broker", but in `QueryScheduler` it is recorded
immediately after the first serialization and can represent a response that is
later replaced with an error response when it exceeds `maxResponseSizeBytes`.
Either move the metric recording to after the final `responseBytes` is
determined, or adjust the description to clarify what is being measured.
```suggestion
* Size of the initially serialized query response in bytes.
* Note: This may differ from the final response actually sent to the
broker if the response
* is later replaced (for example, when exceeding the configured max
response size).
*/
QUERY_RESPONSE_SIZE("bytes", false,
"Size of the initially serialized query response in bytes (may differ
from final response sent to broker)"),
```
##########
pinot-core/src/main/java/org/apache/pinot/core/query/scheduler/QueryScheduler.java:
##########
@@ -163,20 +163,24 @@ protected byte[]
processQueryAndSerialize(ServerQueryRequest queryRequest, Execu
// TODO: Perform this check sooner during the serialization of DataTable.
Map<String, String> queryOptions =
queryRequest.getQueryContext().getQueryOptions();
Long maxResponseSizeBytes =
QueryOptionsUtils.getMaxServerResponseSizeBytes(queryOptions);
- if (maxResponseSizeBytes != null && responseBytes != null &&
responseBytes.length > maxResponseSizeBytes) {
- String errMsg =
- "Serialized query response size " + responseBytes.length + "
exceeds threshold " + maxResponseSizeBytes
- + " for requestId " + requestId + " from broker " +
queryRequest.getBrokerId();
- LOGGER.error(errMsg);
-
_serverMetrics.addMeteredTableValue(queryRequest.getTableNameWithType(),
- ServerMeter.LARGE_QUERY_RESPONSE_SIZE_EXCEPTIONS, 1);
+ if (maxResponseSizeBytes != null && responseBytes != null) {
+ int responseSizeBytes = responseBytes.length;
+ String tableNameWithType = queryRequest.getTableNameWithType();
+ _serverMetrics.addMeteredTableValue(tableNameWithType,
ServerMeter.QUERY_RESPONSE_SIZE, responseSizeBytes);
+ if (responseSizeBytes > maxResponseSizeBytes) {
Review Comment:
`QUERY_RESPONSE_SIZE` is only recorded when `maxResponseSizeBytes != null`,
so queries without this option configured won't emit the new metric. If the
goal is to track response sizes for all queries, record the metric whenever
`responseBytes != null`, and only guard the threshold/exception handling with
the `maxResponseSizeBytes` check.
```suggestion
if (responseBytes != null) {
int responseSizeBytes = responseBytes.length;
String tableNameWithType = queryRequest.getTableNameWithType();
_serverMetrics.addMeteredTableValue(tableNameWithType,
ServerMeter.QUERY_RESPONSE_SIZE, responseSizeBytes);
if (maxResponseSizeBytes != null && responseSizeBytes >
maxResponseSizeBytes) {
```
--
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]
