Copilot commented on code in PR #16576:
URL: https://github.com/apache/pinot/pull/16576#discussion_r2269074522
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/LeafOperator.java:
##########
@@ -671,15 +674,33 @@ public long merge(long value1, long value2) {
public String getStatName() {
return "responseSerializationCpuTimeNs";
}
- };
+ },
+ /**
+ * Allocated memory in bytes for this operator or its children in the same
stage.
+ */
+ ALLOCATED_MEMORY_BYTES(StatMap.Type.LONG, null),
+ /**
+ * Time spent on GC while this operator or its children in the same stage
were running.
+ */
+ GC_TIME_MS(StatMap.Type.LONG, null),
+ // IMPORTANT: When adding new StatKeys, make sure to either create the
same key in BrokerResponseNativeV2.KeyStat or
+ // call the constructor that accepts a String as last argument and set it
to null.
+ // Otherwise the constructor will fail with an IllegalArgumentException
which will not be caught and will
+ // propagate to the caller, causing the query to timeout.
+ ; //@formatter:on
private final StatMap.Type _type;
@Nullable
private final BrokerResponseNativeV2.StatKey _brokerKey;
StatKey(StatMap.Type type) {
_type = type;
- _brokerKey = BrokerResponseNativeV2.StatKey.valueOf(name());
+ try {
+ _brokerKey = BrokerResponseNativeV2.StatKey.valueOf(name());
+ } catch (IllegalArgumentException e) {
+ LOGGER.error("Failed to map StatKey: {} to
BrokerResponseNativeV2.StatKey", name(), e);
+ throw e;
Review Comment:
The error handling logic in the constructor attempts to map StatKey to
BrokerResponseNativeV2.StatKey and logs an error before re-throwing the
exception. However, this approach may not be helpful since the error message
doesn't provide guidance on how to fix the issue. Consider adding a more
descriptive error message that explains the expected resolution (creating the
corresponding key in BrokerResponseNativeV2.KeyStat or using the constructor
with null parameter).
```suggestion
String errorMsg = String.format(
"Failed to map StatKey: %s to BrokerResponseNativeV2.StatKey. " +
"To resolve this, either create the corresponding key in
BrokerResponseNativeV2.KeyStat " +
"or use the StatKey constructor with a null parameter for
brokerKey. " +
"See LeafOperator.StatKey documentation for details.", name());
LOGGER.error(errorMsg, e);
throw new IllegalArgumentException(errorMsg, e);
```
##########
pinot-common/src/main/java/org/apache/pinot/common/datatable/StatMap.java:
##########
@@ -45,7 +45,7 @@
* <li>Change the name of the keys</li>
* </ul>
*
- * Any other change (like changing the type of key, changing their literal
order are not supported or removing keys)
+ * Any other change (like changing the type of key, changing their literal
order or removing keys)
Review Comment:
Missing verb 'are' before 'not supported'. The sentence should read 'Any
other change (like changing the type of key, changing their literal order or
removing keys) are not supported or removing keys) are backward incompatible
changes.' However, this creates a duplicate phrase. Consider revising to: 'Any
other changes (like changing the type of key, changing their literal order, or
removing keys) are backward incompatible changes.'
```suggestion
* Any other changes (like changing the type of key, changing their literal
order, or removing keys)
```
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/MultiStageOperator.java:
##########
@@ -127,7 +134,9 @@ public MseBlock nextBlock() {
nextBlock = ErrorMseBlock.fromException(e);
}
int numRows = nextBlock instanceof MseBlock.Data ? ((MseBlock.Data)
nextBlock).getNumRows() : 0;
- registerExecution(executeStopwatch.elapsed(TimeUnit.MILLISECONDS),
numRows);
+ long memoryUsedBytes = resourceSnapshot.getAllocatedBytes();
+ long gcTimeMs = getGcTimeMillis() - preBlockGcTime;
Review Comment:
The GC time calculation may produce incorrect results when
`isCollectGcStats` is false, as `getGcTimeMillis()` returns -1 in that case.
This would result in `gcTimeMs` being 0 (since -1 - (-1) = 0), but the logic
should consistently return -1 or 0 to indicate disabled collection. Consider
checking the flag before performing the subtraction.
```suggestion
long gcTimeMs;
if (isCollectGcStats()) {
gcTimeMs = getGcTimeMillis() - preBlockGcTime;
} else {
gcTimeMs = -1;
}
```
--
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]
