fix #1727 - Union bySegment queries fix #1730
Conversation
|
@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
force-pushed
the
union-queries-fix
branch
from
3d832ae to
1c2575f
Compare
|
Ah, updated the commit message and PR description to have correct message. |
|
weird failure on that one:
|
| @@ -150,12 +151,12 @@ public int postEventsFromFile(String file) | |||
| ? MediaType.APPLICATION_JSON | |||
| : SmileMediaTypes.APPLICATION_JACKSON_SMILE; | |||
| totalEventsPosted += postEvents(events, mapper, mediaType); | |||
| ; | |||
| expectedEvents += events.size(); | |||
There was a problem hiding this comment.
Is this related to the rest of the PR?
There was a problem hiding this comment.
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.
nishantmonu51
force-pushed
the
union-queries-fix
branch
from
1c2575f to
144b3e5
Compare
|
Looks good although is it possible to add a regression test? |
nishantmonu51
force-pushed
the
union-queries-fix
branch
from
144b3e5 to
8f73e33
Compare
|
@gianm cant add test for the exact same bug since it was in the code path that is now removed. |
nishantmonu51
force-pushed
the
union-queries-fix
branch
from
8f73e33 to
6f76663
Compare
|
@nishantmonu51 looks like one of the unit test is actually failing |
nishantmonu51
force-pushed
the
union-queries-fix
branch
2 times, most recently
from
90df05a to
c720f1e
Compare
|
Ah, i screwed up while squashing commits for a renamed file, should be fixed now. |
|
👍 |
|
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? |
|
After this change, historical nodes will not be aware of how to run union queries. |
|
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? |
|
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? ;) |
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.
|
updated the docs and the PR name. |
nishantmonu51
force-pushed
the
union-queries-fix
branch
from
c720f1e to
573aa96
Compare
fix #1727 - Union bySegment queries fix
|
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 |
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.