Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ML] Add lazy parsing for DatafeedConfig:Aggs,Query #36117

Merged

Conversation

benwtrent
Copy link
Member

@benwtrent benwtrent commented Nov 30, 2018

This adds lazy parsing for DatafeedConfig Aggregations and Queries.

When creating a NEW Datafeed, the STRICT parser is used, which eagerly parses both to catch any formatting errors.

When an already stored config is read in, the LENIENT parser is used and simply pulls in the aggs and datafeed as maps. When serializing on the wire or to XContent, the maps are used directly. However, if the in/out streams are of an earlier version, they are fully parsed before sending over the wire.

Right now the BWC checks are for Version.CURRENT as I am not sure if we want to put this in 6.x. Let me know what you think.

When a lazy parsing issue occurs, I am currently throwing a UNPROCESSABLE_ENTITY REST error. However, the argument could be made that they should just be IllegalArgument exceptions or a XContentParseException, need feedback :).

@elasticmachine
Copy link
Collaborator

Pinging @elastic/ml-core

@droberts195
Copy link
Contributor

I am not sure if we want to put this in 6.x

I'd definitely like to see this in 6.7, because then a mixed version cluster of 7.x and 6.last is guaranteed not to fail due to search or aggs syntax that's been removed in 7.0.

I don't think it would hurt to introduce this into 6.x while it's still 6.6. Since this work is nearly complete I think we should do this, then merge master and 6.x into the corresponding jindex branches and make sure it works in those branches too before merging them to master and 6.x.

Copy link
Contributor

@droberts195 droberts195 left a comment

Choose a reason for hiding this comment

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

LGTM

@benwtrent benwtrent merged commit 166d9a9 into elastic:master Dec 4, 2018
@benwtrent benwtrent deleted the feature/ml-lazy-df-agg-parsing-impl branch December 4, 2018 15:41
benwtrent added a commit that referenced this pull request Dec 4, 2018
* Lazily parsing aggs and query in DatafeedConfigs

* Adding parser tests

* Fixing exception types && unneccessary checked ex

* Adding semi aggregation parser

* Adding tests, fixing up semi-parser

* Reverting semi-parsing

* Moving agg validations

* Making bad configs throw badRequestException
benwtrent added a commit that referenced this pull request Dec 4, 2018
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.

5 participants