Query should not fail because emitter fails or throws Exception#5484
Query should not fail because emitter fails or throws Exception#5484b-slim merged 1 commit intoapache:masterfrom
Conversation
niketh
force-pushed
the
emitter-phaser-issue
branch
from
849c56a to
e9f5658
Compare
gianm
left a comment
gianm
left a comment
There was a problem hiding this comment.
I see this is already merged but please consider fixing the issues anyway. The log message won't be right with the current code.
Also, the try/catch for emitter exceptions is probably indicating some bigger problem. emitter.emit is assumed by lots of places to never fail and Emitter implementations must adhere to that. This fix will help with one specific location, but there are a ton of other places where the emitter is used and exceptions thrown by it will cause problems. I think it's better to fix broken Emitter implementations than to put try/catches everywhere throughout the rest of Druid.
| private final long creationTimeNs; | ||
| private final ObjLongConsumer<? super QueryMetrics<?>> reportMetric; | ||
| private final Consumer<QueryMetrics<?>> applyCustomDimensions; | ||
| private static final Logger log = new Logger(MetricsEmittingQueryRunner.class); |
There was a problem hiding this comment.
Please put the static finals at the top of the file, where they usually go.
| } | ||
| catch (Exception e) { | ||
| // Query should not fail, because of emitter failure. Swallowing the exception. | ||
| log.error("Failure while trying to emit [%s] with stacktrace [%s]", emitter.toString(), e); |
There was a problem hiding this comment.
This should be log.error(e, "Failure emitting query metrics with emitter[%s]", emitter). The exception goes first, or else it won't be interpreted as a stack trace.
There was a problem hiding this comment.
you are right, sorry missed that.
|
I raised #5486 with a javadoc clarifying the Emitter.emit contract that has been implicitly in place. |
|
What about reverting this PR after #5488 is merged? |
5 participants
Isolate emitter failures and don't let them affect query failures #5485