Skip to content

Do not generate msearch request for visualizations that set requiresSearch to false#9658

Merged
ppisljar merged 2 commits intoelastic:masterfrom
nreese:requiresSearch
Feb 20, 2017
Merged

Do not generate msearch request for visualizations that set requiresSearch to false#9658
ppisljar merged 2 commits intoelastic:masterfrom
nreese:requiresSearch

Conversation

@nreese
Copy link
Copy Markdown
Contributor

@nreese nreese commented Dec 27, 2016

Some visualizations do not require aggregation results. These include Markdown and Timelion. Visualizations that do not require aggregation results set the property requiresSearch to false.

Currently, the requiresSearch flag is not utilized when the _msearch request is generated for a dashboard. Visualizations that set the requiresSearch flag to false still generate a search request. Under most situations, this extra request is harmless - other than wasting CPU cycles. When the dashboard contains filters that filter fields not present in the default index, the extra request results in Elasticsearch query_shard_exception as defined in issue 9492.

This PR updates the visualization to only watch the searchSource object (and therefore only create request objects) when the visualizations requires search.

@elasticmachine
Copy link
Copy Markdown
Contributor

Can one of the admins verify this patch?

@Bargs Bargs added Feature:Visualizations Generic visualization features (in case no more specific feature label is available) review labels Dec 27, 2016
@tbragin tbragin requested a review from ppisljar January 5, 2017 13:49
@ppisljar
Copy link
Copy Markdown
Contributor

ppisljar commented Jan 9, 2017

jenkins, test this

@ppisljar
Copy link
Copy Markdown
Contributor

ppisljar commented Jan 9, 2017

Thanks @nreese ! could you also add one test for this ?

@nreese
Copy link
Copy Markdown
Contributor Author

nreese commented Jan 10, 2017

@ppisljar There is no existing test for the ui/public/visualize/visualize.js. How do I bootstrap a test for an angular directive?

@ppisljar
Copy link
Copy Markdown
Contributor

you can create tests folder anywhere you like and just put .js files in it and they will be run during tests.

you can find some info on general angular controller testing here: https://docs.angularjs.org/guide/unit-testing

you can see an example controller test here: https://github.com/elastic/kibana/tree/master/src/ui/public/kbn_top_nav

@ppisljar
Copy link
Copy Markdown
Contributor

jenkins, test this

@ppisljar
Copy link
Copy Markdown
Contributor

needs to be rebased on master for selenium tests to pass.

Copy link
Copy Markdown
Contributor

@ppisljar ppisljar left a comment

Choose a reason for hiding this comment

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

LGTM

@ppisljar
Copy link
Copy Markdown
Contributor

@thomasneirynck do you want to take second look ?

Copy link
Copy Markdown
Contributor

@thomasneirynck thomasneirynck left a comment

Choose a reason for hiding this comment

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

sorry for the delay, this fell of my radar. thanks @nreese , nice addition!

@thomasneirynck
Copy link
Copy Markdown
Contributor

jenkins, test this

@ppisljar
Copy link
Copy Markdown
Contributor

i think this needs to be rebased, CI is failling

@nreese
Copy link
Copy Markdown
Contributor Author

nreese commented Feb 20, 2017

I have rebased with master

@ppisljar ppisljar merged commit 15d1e7c into elastic:master Feb 20, 2017
elastic-jasper added a commit that referenced this pull request Feb 20, 2017
…earch to false

Backports PR #9658

**Commit 1:**
only watch searchSource when vis requires search

* Original sha: 0b38c3c
* Authored by nreese <reese.nathan@gmail.com> on 2016-12-27T19:27:42Z

**Commit 2:**
add test

* Original sha: 592865c
* Authored by nreese <reese.nathan@gmail.com> on 2017-01-12T03:36:04Z
ppisljar pushed a commit that referenced this pull request Feb 20, 2017
…earch to false (#10466)

Backports PR #9658

**Commit 1:**
only watch searchSource when vis requires search

* Original sha: 0b38c3c
* Authored by nreese <reese.nathan@gmail.com> on 2016-12-27T19:27:42Z

**Commit 2:**
add test

* Original sha: 592865c
* Authored by nreese <reese.nathan@gmail.com> on 2017-01-12T03:36:04Z
@ppisljar
Copy link
Copy Markdown
Contributor

thanks a lot @nreese

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

Labels

Feature:Visualizations Generic visualization features (in case no more specific feature label is available) review v5.4.0 v6.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants