Jackie-Jiang commented on code in PR #13035:
URL: https://github.com/apache/pinot/pull/13035#discussion_r1628117693
##########
pinot-common/src/main/java/org/apache/pinot/common/metrics/ServerTimer.java:
##########
@@ -54,7 +54,14 @@ public enum ServerTimer implements AbstractMetrics.Timer {
UPSERT_REMOVE_EXPIRED_PRIMARY_KEYS_TIME_MS("milliseconds", false,
"Total time taken to delete expired primary keys based on metadataTTL or
deletedKeysTTL"),
GRPC_QUERY_EXECUTION_MS("milliseconds", false, "Total execution time of a
successful query over gRPC"),
- UPSERT_SNAPSHOT_TIME_MS("milliseconds", false, "Total time taken to take
upsert table snapshot");
+ UPSERT_SNAPSHOT_TIME_MS("milliseconds", false, "Total time taken to take
upsert table snapshot"),
+
+ // Multi-stage
+ HASH_JOIN_CPU_TIME_BUILDING_HASH_TABLE_MS("millis", true),
Review Comment:
(minor) Consider renaming to `HASH_JOIN_BUILD_TABLE_CPU_TIME_MS` for
consistency
##########
pinot-common/src/main/java/org/apache/pinot/common/metrics/ServerMeter.java:
##########
@@ -118,7 +118,15 @@ public enum ServerMeter implements AbstractMetrics.Meter {
LARGE_QUERY_RESPONSES_SENT("largeResponses", false),
TOTAL_THREAD_CPU_TIME_MILLIS("millis", false),
LARGE_QUERY_RESPONSE_SIZE_EXCEPTIONS("exceptions", false),
- STREAM_DATA_LOSS("streamDataLoss", false);
+ STREAM_DATA_LOSS("streamDataLoss", false),
+
+ // Multi-stage
+ HASH_JOIN_TIMES_MAX_ROWS_REACHED("times", true),
+ AGGREGATE_TIMES_NUM_GROUPS_LIMIT_REACHED("times", true),
+ MULTI_STAGE_IN_MEMORY_MESSAGES("messages", true),
+ MULTI_STAGE_RAW_MESSAGES("messages", true),
+ MULTI_STAGE_RAW_BYTES("bytes", true),
+ MAX_ROWS_IN_WINDOW_REACHED("times", true),;
Review Comment:
(minor) Consider renaming it to `WINDOW_TIMES_MAX_ROWS_REACHED` for
consistency? Also remove the extra comma
##########
pinot-common/src/main/java/org/apache/pinot/common/metrics/ServerTimer.java:
##########
@@ -54,7 +54,14 @@ public enum ServerTimer implements AbstractMetrics.Timer {
UPSERT_REMOVE_EXPIRED_PRIMARY_KEYS_TIME_MS("milliseconds", false,
"Total time taken to delete expired primary keys based on metadataTTL or
deletedKeysTTL"),
GRPC_QUERY_EXECUTION_MS("milliseconds", false, "Total execution time of a
successful query over gRPC"),
- UPSERT_SNAPSHOT_TIME_MS("milliseconds", false, "Total time taken to take
upsert table snapshot");
+ UPSERT_SNAPSHOT_TIME_MS("milliseconds", false, "Total time taken to take
upsert table snapshot"),
+
+ // Multi-stage
+ HASH_JOIN_CPU_TIME_BUILDING_HASH_TABLE_MS("millis", true),
+ MULTI_STAGE_SERIALIZATION_CPU_TIME_MS("millis", true),
+ MULTI_STAGE_DESERIALIZATION_CPU_TIME_MS("millis", true),
+ RECEIVE_DOWNSTREAM_CPU_TIME_MS("millis", true),
+ RECEIVE_UPSTREAM_CPU_WAIT_MS("millis", true),;
Review Comment:
(minor) remove the extra comma
##########
pinot-common/src/main/java/org/apache/pinot/common/metrics/ServerTimer.java:
##########
@@ -54,7 +54,14 @@ public enum ServerTimer implements AbstractMetrics.Timer {
UPSERT_REMOVE_EXPIRED_PRIMARY_KEYS_TIME_MS("milliseconds", false,
"Total time taken to delete expired primary keys based on metadataTTL or
deletedKeysTTL"),
GRPC_QUERY_EXECUTION_MS("milliseconds", false, "Total execution time of a
successful query over gRPC"),
- UPSERT_SNAPSHOT_TIME_MS("milliseconds", false, "Total time taken to take
upsert table snapshot");
+ UPSERT_SNAPSHOT_TIME_MS("milliseconds", false, "Total time taken to take
upsert table snapshot"),
+
+ // Multi-stage
+ HASH_JOIN_CPU_TIME_BUILDING_HASH_TABLE_MS("millis", true),
+ MULTI_STAGE_SERIALIZATION_CPU_TIME_MS("millis", true),
+ MULTI_STAGE_DESERIALIZATION_CPU_TIME_MS("millis", true),
+ RECEIVE_DOWNSTREAM_CPU_TIME_MS("millis", true),
Review Comment:
Receive downstream is confusing. IIUC these 2 are the wait time for mailbox
receive node. Some javadoc can help understand this, or consider renaming them
to `MAILBOX_RECEIVE_DOWNSTREAM_WAIT_CPU_TIME_MS` and
`MAILBOX_RECEIVE_UPSTREAM_WAIT_CPU_TIME_MS`
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/plan/MultiStageQueryStats.java:
##########
@@ -256,7 +256,7 @@ public void mergeUpstream(MultiStageQueryStats otherStats) {
myStats.merge(otherStatsForStage);
}
} catch (IllegalArgumentException | IllegalStateException ex) {
- LOGGER.warn("Error merging stats on stage {}. Ignoring the new stats",
i, ex);
+ LOGGER.warn("Error merging stats on stage " + i + ". Ignoring the new
stats", ex);
Review Comment:
This seems anti-pattern. Do you want to avoid regexp match?
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/plan/MultiStageQueryStats.java:
##########
@@ -284,14 +284,18 @@ public void mergeUpstream(List<ByteBuffer> otherStats) {
myStats.merge(dis);
}
} catch (IOException ex) {
- LOGGER.warn("Error deserializing stats on stage {}. Considering the
new stats empty", i, ex);
+ LOGGER.warn("Error deserializing stats on stage " + i + ".
Considering the new stats empty", ex);
} catch (IllegalArgumentException | IllegalStateException ex) {
- LOGGER.warn("Error merging stats on stage {}. Ignoring the new
stats", i, ex);
+ LOGGER.warn("Error merging stats on stage " + i + ". Ignoring the
new stats", ex);
}
}
}
}
+ public List<StageStats.Closed> getClosedStats() {
+ return Collections.unmodifiableList(_closedStats);
Review Comment:
Since this is internal only code, we can skip wrapping into unmodifiable to
reduce overhead
--
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
