Thread-safe QueryMetrics #6402

gaodayue · 2018-09-29T11:32:09Z

It should be easy to use QueryMetrics from different threads for scenario like JDBC. The doc says "DefaultQueryMetrics's ownerThread should be reassigned explicitly", but it is not clear how best to do that.

I propose to make QueryMetrics thread safe so that it could be used in different threads with minimum effort. One case directly benefiting from the design is that the yielderOpenCloseExecutor hack could be removed. It also solves the above issues instantly.

leventov has pointed out one drawback of making QueryMetrics thread-safe

making QueryMetrics thread-safe is inherently wrong thing to do. Instead of failing with ConcurrentModificationException, multiple threads will override metrics of each other. Nobody would understand what is going on and if the metrics are trustworthy or not.

But I think it's also possible to catch bugs earlier with thread-safe QueryMetrics. For example, DefaultQueryMetrics can log an error if different values are added to the same dimension or metric.

@leventov @gianm @jon-wei what do you think?

leventov · 2018-10-01T13:56:45Z

processing/src/main/java/org/apache/druid/query/GroupByMergedQueryRunner.java

@@ -114,10 +113,10 @@ public Void call()
                          {
                            try {
                              if (bySegment) {
-                                input.run(threadSafeQueryPlus, responseContext)
+                                input.run(queryPlus.withQueryMetricsCopied(), responseContext)


Doesn't this allow silent metric skipping bugs? They could be recorded in the copied queryMetrics, but not finally reported in the "root" queryMetrics.

Yes, it could happen if the client of QueryMetrics forgot to emit. But I think the previous approach has the same problem. And this kind of bugs could be easily found and fixed.

Previously there were no QueryMetrics object in threadSafeQueryPlus, it will throw NPE if somebody tries to emit some metrics by mistake.

OK, I'll revert changes relating to QueryPlus class. However, since QueryMetrics is now a thread-safe class, the old name withoutThreadUnsafeState() is out of date. I will use withoutQueryMetrics() instead.

leventov · 2018-10-01T15:01:36Z

processing/src/main/java/org/apache/druid/query/DefaultQueryMetrics.java

-    builder.setDimension(dimension, value);
+    synchronized (lock) {
+      String oldValue = singleValueDims.put(dimension, value);
+      if (oldValue != null && !oldValue.equals(value)) {


Equal values should not be tolerated. Any reassignment of already existing dimension or metric should be illegal.

ServerManager.buildAndDecorateQueryRunner reassigns segment id in each MetricsEmittingQueryRunner. Do you think we should fix that or just tolerate it? I think allowing for equal values make no harm and is convenient in such cases.

Allowing equal metrics could easily shallow mistakes. Metrics or dimensions could be equal because they happen to collide (especially in tests/staging environment), but logically they are different, that will hit in production.

I agree that allowing equal metrics could shallow mistakes like multiple threads trying to report the same metrics, will prohibit such usage. But I think we should allow equal dimension values because there are valid use cases such as nesting MetricsEmittingQueryRunner. Do you think so?

Could you give a specific example where setting the same dimensions from multiple threads is needed? Both #4826 and #5128 are due to concurrent metrics reporting, not dimensions setting.

I don't want to allow setting dimensions from multiple threads until it proves to be needed.

It's possible to set dimensions from multiple threads. For example, MetricsEmittingQueryRunner adds custom dimension in one thread #L91 and other dimension in possibility another thread #L112.

It's also possible for one thread to set the same dimensions more than once. For instance, the query runner for historical segment processing contains two MetricsEmittingQueryRunner, which would set "segment" and "status" dimension twice.

I don't think there are cases for setting the same dimensions from multiple threads. But the overhead to prohibit such usages is a bit high for me. You needs to maintain the owner thread for each dimension. And I think the checks in reportMetric and emit are sufficient to catch most bugs.

processing/src/main/java/org/apache/druid/query/DefaultQueryMetrics.java

leventov · 2018-10-01T15:03:52Z

processing/src/main/java/org/apache/druid/query/DefaultQueryMetrics.java

+  private void handleIllegalModification(String queryId, String entryName)
+  {
+    Exception e = new Exception("stack trace");
+    log.makeAlert(e, "\"%s\" in QueryMetrics got modified to another value", entryName)


Should throw an IllegalStateException, not just log it. Errors of this kind should remain very verbose.

I didn't throw exception here because I think bugs in metrics handling shouldn't fail the whole query. It would allow users continue to query with only "broken" metrics, which may be acceptable in some use cases. I admit that this may be controversial and would like to hear other committer's opinions.

Because metrics are "non-critical" it's doubly important to propagate errors related to them - otherwise problems with metrics will remain unnoticed for years.

OK, will throw IllegalStateException instead

processing/src/main/java/org/apache/druid/query/DefaultQueryMetrics.java

processing/src/main/java/org/apache/druid/query/QueryMetrics.java

gaodayue · 2018-10-01T17:04:21Z

Thanks @leventov for reviewing! I replied to some of the review comments, please check that.

…trics

gaodayue · 2018-10-08T08:48:29Z

@leventov addressed most of your comments. Could you review it again?

gaodayue · 2018-10-08T11:51:34Z

I think the failed test case RemoteTaskRunnerTest is irrelevant

processing/src/main/java/org/apache/druid/query/DefaultQueryMetrics.java

processing/src/main/java/org/apache/druid/query/QueryMetrics.java

sql/src/test/java/org/apache/druid/sql/calcite/util/SpecificSegmentsQuerySegmentWalker.java

sql/src/test/java/org/apache/druid/sql/calcite/util/CalciteTests.java

leventov · 2018-10-09T16:03:05Z

processing/src/main/java/org/apache/druid/query/DefaultQueryMetrics.java

+  @GuardedBy("lock") private final Map<String, String> singleValueDims = new HashMap<>();
+  @GuardedBy("lock") private final Map<String, String[]> multiValueDims = new HashMap<>();
+  @GuardedBy("lock") private final Map<String, Number> metrics = new HashMap<>();
+  @GuardedBy("lock") private Thread ownerThread;


Should remain protected for subclasses

subclasses are supposed to use setDimension/setDimensions/reportMetric methods (with proper synchronization and check) instead of directly manipulating on these fields

I mean specifically the ownerThread field.

leventov · 2018-10-09T16:11:51Z

processing/src/main/java/org/apache/druid/query/QueryMetrics.java

@@ -92,7 +94,7 @@
 * 100% guarantee of compatibility, because methods could not only be added to QueryMetrics, existing methods could also
 * be changed or removed.
 *
- * <p>QueryMetrics is designed for use from a single thread, implementations shouldn't care about thread-safety.
+ * <p><b>All implementations of QueryMetrics should be Thread-safe.</b>
 *
 *
 * Adding new methods to QueryMetrics


Since the requirements for all methods has changed, probably the name of this class should be changed to "ConcurrentQueryMetrics" to intentionally break proprietary implementations and force them to overhaul.

"DefaultQueryMetrics" OTOH I think could retain the name because it implemented thread-safety itself.

If needed, we can do the renaming in a separate PR. I want to keep this PR small and easy to review.

Change integrity is more important. I would be understandable if the PR was of thousands of lines already, but it's much smaller and will still be relatively small after the rename.

.../src/main/java/org/apache/druid/query/groupby/epinephelinae/GroupByMergingQueryRunnerV2.java

…trics

gaodayue · 2018-10-23T09:33:21Z

@leventov sorry for the late reply. I have addressed previous round of review comments, could you check it again?

leventov · 2018-10-30T14:51:03Z

processing/src/main/java/org/apache/druid/query/DefaultQueryMetrics.java

+      if (ownerThread == null) {
+        ownerThread = currThread;
+      } else if (!ownerThread.equals(currThread)) {
+        throw new ISE("emit() from multiple threads is not allowed. If it is needed to emit metrics from "


Not formatted properly. Concatenated string arg should start at it's own line and in the end ); be on the separate line as well.

leventov · 2018-10-30T14:56:20Z

processing/src/main/java/org/apache/druid/query/GroupByMergedQueryRunner.java

@@ -114,10 +113,10 @@ public Void call()
                          {
                            try {
                              if (bySegment) {
-                                input.run(threadSafeQueryPlus, responseContext)
+                                input.run(queryPlus.withoutQueryMetrics(), responseContext)


It needs to be explained now why queryPlus.withoutQueryMetrics() is passed down. Maybe involves some research why queryPlus.withoutThreadUnsafeState() used to be called in the first place.

leventov · 2018-10-30T15:01:56Z

processing/src/main/java/org/apache/druid/query/QueryMetrics.java

+ *
+ * 1. For all methods that add dimension: if the value of one dimension is updated to another value, throws
+ * IllegalStateException.
+ * 2. For all methods that add metrics: if one metric is regitered more than once, throws IllegalStateException


"registered"

leventov · 2018-10-30T15:05:26Z

processing/src/main/java/org/apache/druid/query/QueryPlus.java

  /**
   * Returns the same QueryPlus object, if it doesn't have {@link QueryMetrics} ({@link #getQueryMetrics()} returns
   * null), or returns a new QueryPlus object with {@link Query} from this QueryPlus and null as QueryMetrics.
+   *
+   * When you pass QueryPlus to multiple threads, it is usually needed to strip off QueryMetrics component first so that
+   * each thread will create its own QueryMetrics. See {@link ChainedExecutionQueryRunner} for an example.


QueryMetrics should be a javadoc link

leventov · 2018-10-31T14:57:52Z

processing/src/main/java/org/apache/druid/query/ChainedExecutionQueryRunner.java

@@ -116,7 +115,7 @@ public ChainedExecutionQueryRunner(
                                public Iterable<T> call()
                                {
                                  try {
-                                    Sequence<T> result = input.run(threadSafeQueryPlus, responseContext);
+                                    Sequence<T> result = input.run(queryPlus.withoutQueryMetrics(), responseContext);


Ok I see, there is at least one reason for doing this, is to not emit queryCpuTime metrics twice, see #1696 (comment). It works relying on a crazy combination of side effects, without a single comment. It requires a refactoring.

stale · 2019-02-28T05:14:47Z

This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@druid.apache.org list. Thank you for your contributions.

stale · 2019-03-07T06:09:50Z

This pull request has been closed due to lack of activity. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.

gaodayue added 2 commits September 29, 2018 17:07

require QueryMetrics to be thread-safe

394e53c

log incorrect use of QueryMetrics to catch bugs earlier

5754c11

leventov reviewed Oct 1, 2018

View reviewed changes

leventov added Area - Metrics/Event Emitting Design Review labels Oct 1, 2018

leventov requested changes Oct 1, 2018

View reviewed changes

gaodayue added 2 commits October 8, 2018 16:35

prohibit emit() from multiple threads

9938640

Merge remote-tracking branch 'apache/master' into threadsafe-query-me…

0fcfa6e

…trics

leventov requested changes Oct 9, 2018

View reviewed changes

gaodayue added 3 commits October 23, 2018 14:21

Merge remote-tracking branch 'apache/master' into threadsafe-query-me…

5d1d3bd

…trics

address review comments about formatting and doc

4f59c23

fix test

e191ed3

leventov reviewed Oct 30, 2018

View reviewed changes

leventov reviewed Oct 31, 2018

View reviewed changes

stale bot added the stale label Feb 28, 2019

stale bot closed this Mar 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Thread-safe QueryMetrics #6402

Thread-safe QueryMetrics #6402

gaodayue commented Sep 29, 2018 •

edited

Loading

leventov Oct 1, 2018

gaodayue Oct 1, 2018

leventov Oct 2, 2018

gaodayue Oct 8, 2018

leventov Oct 1, 2018

gaodayue Oct 1, 2018

leventov Oct 2, 2018

gaodayue Oct 8, 2018

leventov Oct 9, 2018

gaodayue Oct 23, 2018

gaodayue Oct 23, 2018

leventov Oct 1, 2018

gaodayue Oct 1, 2018

leventov Oct 2, 2018

gaodayue Oct 8, 2018

gaodayue commented Oct 1, 2018

gaodayue commented Oct 8, 2018

gaodayue commented Oct 8, 2018

leventov Oct 9, 2018

gaodayue Oct 23, 2018

leventov Oct 30, 2018

leventov Oct 9, 2018

gaodayue Oct 23, 2018

leventov Oct 30, 2018

gaodayue commented Oct 23, 2018

leventov Oct 30, 2018

leventov Oct 30, 2018

leventov Oct 30, 2018

leventov Oct 30, 2018

leventov Oct 31, 2018

stale bot commented Feb 28, 2019

stale bot commented Mar 7, 2019

Thread-safe QueryMetrics #6402

Thread-safe QueryMetrics #6402

Conversation

gaodayue commented Sep 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaodayue commented Oct 1, 2018

gaodayue commented Oct 8, 2018

gaodayue commented Oct 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaodayue commented Oct 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stale bot commented Feb 28, 2019

stale bot commented Mar 7, 2019

gaodayue commented Sep 29, 2018 •

edited

Loading