Skip to content

fix #1727 - Union bySegment queries fix#1730

Merged
drcrallen merged 1 commit intoapache:masterfrom
metamx:union-queries-fix
Sep 29, 2015
Merged

fix #1727 - Union bySegment queries fix#1730
drcrallen merged 1 commit intoapache:masterfrom
metamx:union-queries-fix

Conversation

@nishantmonu51
Copy link
Copy Markdown
Member

@nishantmonu51 nishantmonu51 commented Sep 14, 2015

Fixes #1727
revert to doing merging for results for union queries on broker.
Historical node will throw Unsupported Exception for Union Queries sent directly to it.

@xvrl
Copy link
Copy Markdown
Member

xvrl commented Sep 14, 2015

@nishantmonu51 I think you mean to say fix #1727, not 1721. I fixed the description, but might want to fix commit message as well

@nishantmonu51 nishantmonu51 changed the title fix #1721 fix #1727 Sep 15, 2015
@nishantmonu51
Copy link
Copy Markdown
Member Author

nishantmonu51 commented Sep 15, 2015

Ah, updated the commit message and PR description to have correct message.

@drcrallen
Copy link
Copy Markdown
Contributor

drcrallen commented Sep 15, 2015

weird failure on that one:

AnnouncerTest.testSessionKilled:157 expected null, but was:<10,10,1442324913450,1442324913450,0,0,0,94524205367558145,25,0,10>

gianm
gianm reviewed Sep 15, 2015
View reviewed changes
Comment thread integration-tests/src/main/java/io/druid/testing/clients/EventReceiverFirehoseTestClient.java Outdated
Copy link
Copy Markdown
Contributor

@gianm gianm Sep 15, 2015

Choose a reason for hiding this comment

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

Is this related to the rest of the PR?

Copy link
Copy Markdown
Member Author

@nishantmonu51 nishantmonu51 Sep 15, 2015

Choose a reason for hiding this comment

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

not related to this PR, it was an integration-test issue, that popped up when i tried to run integration-tests. will separate test fix in a separate PR.

@gianm
Copy link
Copy Markdown
Contributor

gianm commented Sep 15, 2015

Looks good although is it possible to add a regression test?

@drcrallen drcrallen added this to the 0.8.2 milestone Sep 22, 2015
@nishantmonu51
Copy link
Copy Markdown
Member Author

nishantmonu51 commented Sep 22, 2015

@gianm cant add test for the exact same bug since it was in the code path that is now removed.
Added test for UnionQueryRunner that verifies its functionality.

@xvrl
Copy link
Copy Markdown
Member

xvrl commented Sep 22, 2015

@nishantmonu51 looks like one of the unit test is actually failing

@nishantmonu51 nishantmonu51 force-pushed the union-queries-fix branch 2 times, most recently from 90df05a to c720f1e Compare September 22, 2015 22:28
@nishantmonu51
Copy link
Copy Markdown
Member Author

nishantmonu51 commented Sep 22, 2015

Ah, i screwed up while squashing commits for a renamed file, should be fixed now.

@fjy
Copy link
Copy Markdown
Contributor

fjy commented Sep 23, 2015

👍

@drcrallen
Copy link
Copy Markdown
Contributor

drcrallen commented Sep 24, 2015

In a world where I have only one broker and one historical, the desired behavior is that if I submit a query to the broker I get the same result as if I submit it to the historical. Does this break that behavior?

@nishantmonu51
Copy link
Copy Markdown
Member Author

nishantmonu51 commented Sep 24, 2015

After this change, historical nodes will not be aware of how to run union queries.
you will need to send the query to broker always.
there are alternative approaches where we can make this working for historical nodes too. (discussed here - #1727)

@drcrallen
Copy link
Copy Markdown
Contributor

drcrallen commented Sep 29, 2015

Can you update the docs for http://druid.io/docs/latest/querying/datasource.html to indicate the limitations of the Union Query being issued against a broker?

@cheddar
Copy link
Copy Markdown
Contributor

cheddar commented Sep 29, 2015

We talked about this in the dev sync and while it is unfortunate that historicals won't know about the Union query, it is probably the least of evils at this point in time, so I'm 👍 on that aspect (haven't actually verified the code)

Can we update this PR to have a more meaningful name though? ;)

@nishantmonu51
fix apache#1727 - Union bySegment queries fix
573aa96 
Fixes apache#1727.
revert to doing merging for results for union queries on broker.

revert unrelated changes

Add test for union query runner

Add test

remove unused imports

fix imports

fix renamed file

fix test

update docs.
@nishantmonu51 nishantmonu51 changed the title fix #1727 fix #1727 - Union bySegment queries fix Sep 29, 2015
@nishantmonu51
Copy link
Copy Markdown
Member Author

nishantmonu51 commented Sep 29, 2015

updated the docs and the PR name.

drcrallen added a commit that referenced this pull request Sep 29, 2015
@drcrallen
Merge pull request #1730 from metamx/union-queries-fix
2d847ad 
fix #1727 - Union bySegment queries fix
@drcrallen drcrallen merged commit 2d847ad into apache:master Sep 29, 2015
@drcrallen drcrallen deleted the union-queries-fix branch September 29, 2015 19:23
@drcrallen
Copy link
Copy Markdown
Contributor

drcrallen commented Oct 17, 2015

Encountered error during rollout internally related to historicals getting union queries. Investigating more, but this might require BROKERs to be updated before HISTORICALs for 0.8.2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

0.8.2

Development

Successfully merging this pull request may close these issues.

6 participants

@nishantmonu51 @xvrl @drcrallen @gianm @fjy @cheddar