force eager the processing of segment metadata query on the processing executor #1172
Conversation
| @@ -146,7 +147,9 @@ public SegmentMetadataQueryRunnerFactory( | |||
| @Override | |||
| public Sequence<SegmentAnalysis> call() throws Exception | |||
| { | |||
| return input.run(query, responseContext); | |||
There was a problem hiding this comment.
for other query types, ChainedExecutionQueryRunner takes care this by materializing the result, can we use that here also ?
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
i meant instead of duplicating the code for materializing the result at multiple places, can we reuse the ChainedExecutionQueryRunner here ?
There was a problem hiding this comment.
@nishantmonu51 that might be a good idea, I will see if we can use ChainedExecutionQueryRunner here as well.
|
Why is eager processing needed for segment metadata queries? |
|
wait nvmnd, I'm reading the original commit message clearer now.. query runners |
|
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) |
|
@drcrallen most of them seems to be using ChainedExecutionQueryRunner which does the eager materialization. |
|
@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
force-pushed
the
segment_metadata_eager
branch
from
4a87374
to
a2bf98f
Compare
…g threadpool by using ChainedExecutionQueryRunner in SegmentMetadataQueryRunnerFactory.mergeRunners(..)
himanshug
force-pushed
the
segment_metadata_eager
branch
from
a2bf98f
to
55ebf0c
Compare
|
@drcrallen @nishantmonu51 switched to using chained query execution runner |
|
Holy crap that's much cleaner. |
|
closeing/reopening to restart the failed build. |
|
+1 |
force eager the processing of segment metadata query on the processing executor
|
this is causing ITTwitterQueryTest in integration-tests to fail with ClassCastException while fetching bySegmentResults on historical nodes with ClassCastException. |
|
@himanshug it fails when we issue the SegmentMetadataQuery by setting {"bySegment" : true} in context with exception - can you have a look at the changes again ? |
|
@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. |
|
probably dint ran integration tests then, reverting the changes in this PR makes the test pass. |
|
I am checking |
|
caught up into something else, will look at it in 1-2 hrs. let me know if it is blocking u guys or something |
|
@nishantmonu51 turns out switching to use ChainedExecutionQueryRunner doesn't seem to be working well for SegmentMetadata queries :( |
Fixing the regression caused by #1172
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.