force eager the processing of segment metadata query on the processing executor

[himanshug]

This makes sure that, irrespective of the behavior of input query runners, query runner returned by mergeRunners returns a query runner which ensures that processing really happens inside the threadpool of processor executor service instead of jetty threads which is what was intended probably.

[nishantmonu51]

for other query types, ChainedExecutionQueryRunner takes care this by materializing the result, can we use that here also ?

[himanshug]

@nishantmonu51 ChainedExecutionQueryRunner seems to be doing same. see here
https://github.com/druid-io/druid/blob/master/processing/src/main/java/io/druid/query/ChainedExecutionQueryRunner.java#L130

pls let me know if you're suggesting something different?

[nishantmonu51]

i meant instead of duplicating the code for materializing the result at multiple places, can we reuse the ChainedExecutionQueryRunner here ?

[himanshug]

@nishantmonu51 that might be a good idea, I will see if we can use ChainedExecutionQueryRunner here as well.

[drcrallen]

Why is eager processing needed for segment metadata queries?

[drcrallen]

wait nvmnd, I'm reading the original commit message clearer now.. query runners

[drcrallen]

A larger question is, why is the jetty thread doing the materialization here but not elsewhere? (I haven't looked much into this codeline before)

[himanshug]

@drcrallen most of them seems to be using ChainedExecutionQueryRunner which does the eager materialization.
GroupByQueryRunnerFactory does a input.run(..).accumulate(..) which would do the materialization.

[drcrallen]

@himanshug Thanks, in that case I agree with @nishantmonu51 that we should try to keep the methodology consistent for materialization (i.e. use ChainedExecutionQueryRunner) if possible.

[himanshug]

@drcrallen @nishantmonu51 switched to using chained query execution runner

[drcrallen]

Holy crap that's much cleaner.

[himanshug]

closeing/reopening to restart the failed build.

[xvrl]

+1

[nishantmonu51]

this is causing ITTwitterQueryTest in integration-tests to fail with ClassCastException while fetching bySegmentResults on historical nodes with ClassCastException.

[nishantmonu51]

@himanshug it fails when we issue the SegmentMetadataQuery by setting {"bySegment" : true} in context with exception -
java.lang.ClassCastException: io.druid.query.Result cannot be cast to io.druid.query.metadata.metadata.SegmentAnalysis
at io.druid.query.metadata.SegmentMetadataQueryQueryToolChest$4.compare(SegmentMetadataQueryQueryToolChest.java:222) ~[druid-processing-0.7.3-SNAPSHOT.jar:0.7.3-SNAPSHOT]
at com.google.common.collect.NullsFirstOrdering.compare(NullsFirstOrdering.java:44) ~[guava-16.0.1.jar:?]
at com.metamx.common.guava.MergeIterator$1.compare(MergeIterator.java:46) ~[java-util-0.27.0.jar:?]
at com.metamx.common.guava.MergeIterator$1.compare(MergeIterator.java:42) ~[java-util-0.27.0.jar:?]
at java.util.PriorityQueue.siftUpUsingComparator(PriorityQueue.java:649) ~[?:1.7.0_80]

can you have a look at the changes again ?

[himanshug]

@nishantmonu51 this code was added before 0.7.1.1, did we run integration tests back then and they passed or did we not run the tests at that time or is this a nwe test? just trying to understand what changed.

[nishantmonu51]

probably dint ran integration tests then, reverting the changes in this PR makes the test pass.

[himanshug]

I am checking

[himanshug]

caught up into something else, will look at it in 1-2 hrs. let me know if it is blocking u guys or something

[himanshug]

@nishantmonu51 turns out switching to use ChainedExecutionQueryRunner doesn't seem to be working well for SegmentMetadata queries :(
It works for TopN bcoz result type of TopN queries is "Result" which is same as BySegmentQueryRunner, same does not hold true for GroupBy queries and that also did not use ChainedExecutionQueryRunner
Reverting the use of ChainedExecutionQueryRunner here fixes the issue.