Refactor QueryRunner.run(), make responseContext immutable

[leventov]

Mutable Map, passed into QueryRunner.run(), is abused: #4112 and inherently broken with async/concurrent runners: #3803.

I suggest to change QueryRunner signature to

  QueryResult<T> run(Query<T> query);

where

class QueryResult<T>
{
  final Sequence<T> result;
  final ImmutableMap<String, Object> responseContext;
}

What do you think? @himanshug @gianm

[gianm]

I guess that means the response context would need to be returned before queries are finished executing, since they won't be done by the time QueryRunner.run returns. Is that ok?

Example: at one point I was thinking we could use the response context for a possible query profiling feature, something that doesn't involve emitting metrics but instead returns a profile in the response itself. That would probably be tough to do with the change you proposed here, since not all profiling info is known at the time QueryRunner.run returns. I don't plan to work on this feature in the near future though, it was just something I thought might be cool.

Also, I think the fact that CachingClusteredClient merges sequences from servers would mean it would have to know how to merge response contexts too.

[gianm]

#3803 does mean that response contexts are not very useful right now, so I agree something needs to change.

[gianm]

Would it be too weird to have:

class QueryResult<T>
{
  final Sequence<T> result;
  final Future<ImmutableMap<String, Object>> responseContext;
}

Where responseContext is a future that might not be available until the result has finished accumulating?

[gianm]

@leventov in #3803 you said you were using response contexts for more things -- what were they? Some use cases would illuminate what the behavior should be. Right now, in Druid response contexts aren't used for very much so it's not really clear to me what the best behavior is.

[leventov]

I guess that means the response context would need to be returned before queries are finished executing, since they won't be done by the time QueryRunner.run returns. Is that ok?

Yes, I forgot that sometimes responseContext is updated during the Sequence accumulating. Also AFAIR sometimes it is somehow used before sequence is accumulated. So either we need to rethink some logic using responseContext now, or have two responseContexts: "before" and "after", that would be too complex IMO.

In #3803 I was using it to pass what is now known as QueryMetrics. However I should have used Query context for this, or add a specific QueryMetrics property to Query. So I was abusing responseContext as well as in #4112.

[himanshug]

#3803 definitely needs to be fixed

responseContext has very lightweight usage. one option is to change to QueryRunner.run(Query query, ResponseContext context) and ResponseContext class takes care of concurrency where necessary, this might be more straightforward than having

QueryResult {
   ImmutableMap preContext;
   Sequence result;
   Future<ImmutableMap> postContext;
}

@gianm I have tried to do the "profiling" stuff in #3371

[leventov]

@himanshug so we are suggesting the same thing. I raised concern that this design is overly complex, but it might be unavoidable, if we genuinely need preContext and postContext sometimes.

[gianm]

IMO, changes to response context should be motivated by use cases. There is no need to add complexity for things we might need but don't actually need. I suggest we combine this issue with #3803, they seem to be the same underlying problem.

Right now, response context is used for:

• tracking missing segments for query retries (see SpecificSegmentQueryRunner, ReportTimelineMissingSegmentQueryRunner, RetryQueryRunner)
• returning a list of uncoveredIntervals to the client (see Add "uncoveredIntervals" to responseContext #2058, made optional in make populateUncoveredIntervals a configuration in query context #2332)
• tracking timeouts in the Scan query (although this usage is a bit dubious, see ScanQueryRunnerFactory should pass CTX_TIMEOUT_AT via Query context rather than responseContext #4112)

That's all I can think of, maybe there's more that I missed. Possible future usages include query profiling: #4113 (comment) and #3371.

Anyone aware of anything else?

[himanshug]

response context is also used for tracking the "etag" field to implement #3955

[leventov]

@gianm the motivation for change is that it's impossible to reason about correctness with the current design. Probably it's ok right now, but probably not OK already, we may silently miss some queryContext entries and never notice that. It also may depend on the cluster deployment scheme. E. g. if brokers and historicals are deployed on the same nodes.

Generally, shared mutable response context breaks QueryRunner isolation, some combinations of QueryRunners work, but some will definitely not.

I support the following design:

QueryResult {
   ImmutableMap preResponseContext;
   Sequence result;
   Future<ImmutableMap> postResponseContext;
}

IMO the change should be combined with #4131.

[stale]

This issue has been marked as stale due to 280 days of inactivity. It will be closed in 2 weeks if no further activity occurs. If this issue is still relevant, please simply write any comment. Even if closed, you can still revive the issue at any time or discuss it on the dev@druid.apache.org list. Thank you for your contributions.

[stale]

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