Skip to content

refactor ClientQuerySegmentWalker#2837

Merged
fjy merged 3 commits intoapache:masterfrom
jisookim0513:refactor-client-query-segment-walker
Apr 18, 2016
Merged

refactor ClientQuerySegmentWalker#2837
fjy merged 3 commits intoapache:masterfrom
jisookim0513:refactor-client-query-segment-walker

Conversation

@jisookim0513
Copy link
Copy Markdown
Contributor

I have been working on using Spark for long-running Druid queries. I was able to run a long-running druid query without timeout by having intermediate-level brokers that process the query for a specific chunk of segments and letting the final broker merge the query results from intermediate brokers. While doing so, I had to write my own query runners where one does stuff from RetryQueryRunner to toolChest.mergeResults and the other does stuff from toolChest.mergeResults to FinalizeResultsQueryRunner. @xvrl suggested refactoring ClientQuerySegmentWalker this way so I can use the methods from a new class instead of reusing the code from ClientQuerySegmentWalker.

@@ -0,0 +1,123 @@
package io.druid.server;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing header

@jaehc
Copy link
Copy Markdown
Contributor

jaehc commented Apr 14, 2016

This looks great.

@xvrl
Copy link
Copy Markdown
Member

xvrl commented Apr 14, 2016

I'm of course 👍 on this one, but since I was involved I'd prefer if we get two other +1 on this one

@drcrallen
Copy link
Copy Markdown
Contributor

👍 needs 1 more

@b-slim
Copy link
Copy Markdown
Contributor

b-slim commented Apr 15, 2016

LGTM, It will be nice if we add some unit tests to cover the new class.
BTW IT is not a blocking issue.

@nishantmonu51
Copy link
Copy Markdown
Member

👍 , LGTM, @jisookim0513: minor nit, could you also modify QueryRunnerTestHelper.makeXXXQueryRunner to use the new api ?

@jisookim0513
Copy link
Copy Markdown
Contributor Author

@nishantmonu51 sure, I will modify QueryRunnerTestHelper to use the new API.

@gianm
Copy link
Copy Markdown
Contributor

gianm commented Apr 18, 2016

👍

@jisookim0513
Copy link
Copy Markdown
Contributor Author

@nishantmonu51: modified QueryRunnerTestHelper and refactored FluentQueryRunnerBuiler not to take query as a constructor argument (also moved it to druid.io.processing from druid.io.server)

@xvrl
Copy link
Copy Markdown
Member

xvrl commented Apr 18, 2016

👍 on the latest changes.

@fjy fjy merged commit 7b65ca7 into apache:master Apr 18, 2016
seoeun25 pushed a commit to seoeun25/incubator-druid that referenced this pull request Jan 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants