Skip to content

Run GroupByV2 queries in compatibility mode on Historicals#4022

Closed
niketh wants to merge 1 commit intoapache:masterfrom
niketh:hist-compatibility
Closed

Run GroupByV2 queries in compatibility mode on Historicals#4022
niketh wants to merge 1 commit intoapache:masterfrom
niketh:hist-compatibility

Conversation

@niketh
Copy link
Copy Markdown
Contributor

@niketh niketh commented Mar 8, 2017

Run GroupByV2 queries in compatibility mode on Historicals. If the broker doesn't send "groupByStrategy" in the context

@niketh niketh changed the title Hist compatibility Run GroupByV2 queries in compatibility mode on Historicals Mar 8, 2017
@niketh
Copy link
Copy Markdown
Contributor Author

niketh commented Mar 8, 2017

@gianm Can you review

@niketh niketh force-pushed the hist-compatibility branch from 2ebfdb2 to 36ba30d Compare March 8, 2017 13:58
@gianm
Copy link
Copy Markdown
Contributor

gianm commented Mar 8, 2017

I don't think this will help: the compatibility mode in this patch just changes how results are compared when merging, which wouldn't solve the issue on 0.9.1.1 brokers. They'll still get null timestamps and still not be able to handle them. If we want 0.9.1.1 brokers to work with 0.10.0 historicals then the historicals behavior needs to change.

Looking into it a little more, we kind of already have a natural 'compatibility flag' the historicals can use to detect what behavior they should do: CTX_KEY_OUTERMOST, which is set to "false" by new brokers but is null from older brokers. Most of the code to make this work is already there, the only missing piece was that GROUP_BY_MERGE_KEY was being used by both engines to disable mergeResults, but it really should only be used by the v1 engine. v2 should always merge results.

I raised #4023 to do this, @niketh could you take a look please?

@niketh niketh closed this Mar 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants