You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2021/11/03 00:56:08 UTC
[GitHub] [druid] jihoonson commented on a change in pull request #11822: Code cleanup from query profile project
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org