jihoonson commented on a change in pull request #11822:
URL: https://github.com/apache/druid/pull/11822#discussion_r741556524
##########
File path: processing/src/main/java/org/apache/druid/query/QueryMetrics.java
##########
@@ -262,36 +265,58 @@
/**
* Registers "query time" metric.
+ *
+ * Measures the time between a server thread starting to handle a query, and
the response being fully written to
+ * the response output stream. Does not include time spent waiting in the
server queue to acquire a thread.
*/
QueryMetrics<QueryType> reportQueryTime(long timeNs);
/**
* Registers "query bytes" metric.
+ *
+ * Measures the total number of bytes written by the query server thread to
the response output stream.
+ *
+ * Emitted once per query.
*/
QueryMetrics<QueryType> reportQueryBytes(long byteCount);
/**
- * Registeres "segments queried count" metric.
+ * Registers "segments queried count" metric.
*/
QueryMetrics<QueryType> reportQueriedSegmentCount(long segmentCount);
/**
* Registers "wait time" metric.
+ *
+ * Measures the total time segment-processing runnables spend waiting for
execution in the processing thread pool.
+ *
+ * Emitted once per segment.
*/
QueryMetrics<QueryType> reportWaitTime(long timeNs);
/**
* Registers "segment time" metric.
+ *
+ * Measures the total wall-clock time spent operating on segments in
processing threads.
+ *
+ * Emitted once per segment.
*/
QueryMetrics<QueryType> reportSegmentTime(long timeNs);
/**
* Registers "segmentAndCache time" metric.
+ *
+ * Measures the total wall-clock time spent in processing threads, either
operating on segments or retrieving items
+ * from cache.
+ *
+ * Emitted once per segment.
*/
QueryMetrics<QueryType> reportSegmentAndCacheTime(long timeNs);
/**
* Registers "cpu time" metric.
+ *
+ * Measures the total amount of CPU time spent by all threads (server and
processing) involved in a query.
Review comment:
Will it be clearer if you mention that this metric includes segment
processing wait time, segment processing time, and per-segment result merge
time? I guess `server` seems a bit ambiguous to me since there are so many
severs in druid.
##########
File path: processing/src/main/java/org/apache/druid/query/QueryMetrics.java
##########
@@ -32,47 +32,48 @@
* Abstraction wrapping {@link
org.apache.druid.java.util.emitter.service.ServiceMetricEvent.Builder} and
allowing to
* control what metrics are actually emitted, what dimensions do they have,
etc.
*
- *
- * Goals of QueryMetrics
- * ---------------------
- * 1. Skipping or partial filtering of particular dimensions or metrics
entirely. Implementation could leave the body
- * of the corresponding method empty, or implement random filtering like:
+ * <h4>Goals of QueryMetrics</h4>
Review comment:
nit: we don't publish javadoc, and thus prefer a format that is easily
readable in IDEs than in web browsers.
##########
File path: processing/src/main/java/org/apache/druid/segment/data/Indexed.java
##########
@@ -26,6 +26,13 @@
import javax.annotation.Nullable;
import java.util.Comparator;
+/**
+ * Indexed is a fixed-size, immutable, indexed set of values which allows
Review comment:
:+1: thanks for adding javadoc!
##########
File path: processing/src/main/java/org/apache/druid/query/QueryMetrics.java
##########
@@ -262,36 +265,58 @@
/**
* Registers "query time" metric.
+ *
+ * Measures the time between a server thread starting to handle a query, and
the response being fully written to
+ * the response output stream. Does not include time spent waiting in the
server queue to acquire a thread.
Review comment:
Could it be clearer if you say the server thread is a Jetty server
thread?
##########
File path:
core/src/main/java/org/apache/druid/java/util/common/guava/ParallelMergeCombiningSequence.java
##########
@@ -312,14 +314,15 @@ private MergeCombinePartitioningAction(
this.cancellationGizmo = cancellationGizmo;
}
+ @SuppressWarnings("unchecked")
Review comment:
nit: I was looking for where the unchecked operation is used and
realized that it is where `TERMINAL` is added to a queue. Could it be better to
add this suppression at the line where unchecked operation is actually OK such
as adding `TERMINAL` to a queue? The current change seems fine for now but I
suppose this warning can do a legit thing if someone modifies some code in this
method to use an unchecked operation. This comment is not a blocker.
##########
File path: processing/src/main/java/org/apache/druid/query/QueryMetrics.java
##########
@@ -262,36 +265,58 @@
/**
* Registers "query time" metric.
+ *
+ * Measures the time between a server thread starting to handle a query, and
the response being fully written to
+ * the response output stream. Does not include time spent waiting in the
server queue to acquire a thread.
*/
QueryMetrics<QueryType> reportQueryTime(long timeNs);
/**
* Registers "query bytes" metric.
+ *
+ * Measures the total number of bytes written by the query server thread to
the response output stream.
+ *
+ * Emitted once per query.
*/
QueryMetrics<QueryType> reportQueryBytes(long byteCount);
/**
- * Registeres "segments queried count" metric.
+ * Registers "segments queried count" metric.
*/
QueryMetrics<QueryType> reportQueriedSegmentCount(long segmentCount);
/**
* Registers "wait time" metric.
+ *
+ * Measures the total time segment-processing runnables spend waiting for
execution in the processing thread pool.
Review comment:
```suggestion
* Measures the total time segment-processing runnables spent waiting for
execution in the processing thread pool.
```
--
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]
