Sometimes EventListener.queryCompleted() called too early #20231
Conversation
| @@ -22,6 +22,7 @@ | |||
| import io.opentelemetry.api.trace.Tracer; | |||
| import io.opentelemetry.context.Context; | |||
| import io.trino.Session; | |||
There was a problem hiding this comment.
Please extend commit message, e.g: what was the problem?
There was a problem hiding this comment.
It is described in the PR description, but I now copied it to the commit extended message.
| @@ -58,7 +57,6 @@ public FailedDispatchQuery createFailedDispatchQuery(Session session, String que | |||
| BasicQueryInfo queryInfo = failedDispatchQuery.getBasicQueryInfo(); | |||
|
|
|||
| queryMonitor.queryCreatedEvent(queryInfo); | |||
| queryMonitor.queryImmediateFailureEvent(queryInfo, toFailure(throwable)); | |||
There was a problem hiding this comment.
Why having it here was causing race condition? queryCreatedEvent is called just before anyway.
Why query has to be registered via queryTracker.addQuery?
There was a problem hiding this comment.
I have provided relevant information in the extended commit message, like you asked in the other comment. Let me know whether it's clear now. If not, I will try again to describe it better.
There was a problem hiding this comment.
Consider adding QueryTracker as an arg to createFailedDispatchQuery. This way you don't have to add queryMonitor to DispatchManager
dominikzalewski
force-pushed
the
query_completed_too_early
branch
from
54cc3a6 to
bc8163a
Compare
dominikzalewski
changed the title
| @@ -236,6 +241,7 @@ private <C> void createQueryInternal(QueryId queryId, Span querySpan, Slug slug, | |||
| Optional<String> preparedSql = Optional.ofNullable(preparedQuery).flatMap(PreparedQuery::getPrepareSql); | |||
| DispatchQuery failedDispatchQuery = failedDispatchQueryFactory.createFailedDispatchQuery(session, query, preparedSql, Optional.empty(), throwable); | |||
| queryCreated(failedDispatchQuery); | |||
| queryMonitor.queryImmediateFailureEvent(failedDispatchQuery.getBasicQueryInfo(), toFailure(throwable)); | |||
There was a problem hiding this comment.
LGTM but add a comment whgy this is not happening within createFailedDispatchQuery for future readers.
There was a problem hiding this comment.
Comment added. Let me know if this is what you meant and whether I can do anything else to get an approval?
For happy path and parsable queries, we have the following order of calls: - query is added to the QueryTracker - EventListener.queryCompleted() is called For really bad queries, the ones that do not even parse to a proper SQL, the order is inverted. As a result, when implementing an EventListener that wants to access query detailed information we have to use a workaround for this edge case and artificially invert the sequence of calls. EventListener will typically use a retry mechanism combined with switching to a separate thread, which adds unnecessary complexity and multi-threading to a code that otherwise would be single-threaded.
dominikzalewski
force-pushed
the
query_completed_too_early
branch
from
bc8163a to
7b8bafb
Compare
Description
For really bad queries, the ones that do not even parse to a proper SQL, we have the following sequence of calls:
EventListener.queryCompleted()QueryTrackerTo work around incorrect sequence of these calls, an
EventListenerwill typically use a retry mechanism combined with switching to a separate thread.This PR inverts the sequence of calls so that
EventListeners can now be coded single-threaded.Additional context and related issues
Release notes
( X ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text: