leventov commented on a change in pull request #6402: Thread-safe QueryMetrics URL: https://github.com/apache/incubator-druid/pull/6402#discussion_r222071676
########## File path: processing/src/main/java/org/apache/druid/query/DefaultQueryMetrics.java ########## @@ -289,12 +320,26 @@ public void identity(String identity) } @Override - public void emit(ServiceEmitter emitter) + public void emit(final ServiceEmitter emitter) Review comment: This is the corner stone of why I originally made QueryMetrics single-threaded. I agree that the ability to add dimensions and metrics from parallel threads is convenient, but allowing to `emit()` from concurrent threads is paving the road to hell of literally undiscoverable and undebuggable bugs with metrics. All exceptions related to QueryMetrics that have been floating up since the introduction of this class represent our own misunderstanding of the execution flow of some queries - how and when data is transferred between threads, in what specific thread pool some code is actually executed, etc. QueryMetrics force maintainers to refine the threading and make data transitions more explicit, that is a good thing. Making QueryMetrics permissive is easier but it's like burying head in the sand. Where do you need parallel `emit()` and parallel `report()` is not enough? ---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: [email protected] With regards, Apache Git Services --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
