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

Improve robustness of QuickValues with stacked fields #4322

Merged
merged 4 commits into from Nov 7, 2017
Merged

Conversation

@bernd
Copy link
Member

@bernd bernd commented Nov 7, 2017

  • Disable the stacked field form in QuickValues configuration when Elasticsearch version is < 5
  • Reset loading state in case of an error so the user can change settings and try again
  • Reset QuickValues visualization config to defaults when analyzing a new field

Closes #4289

Note: This needs to go into the 2.4 branch as well

@bernd bernd added this to the 2.4.0 milestone Nov 7, 2017
@bernd bernd requested a review from edmundoa Nov 7, 2017
@ghost ghost assigned bernd Nov 7, 2017
@edmundoa edmundoa assigned edmundoa and unassigned bernd Nov 7, 2017
Copy link
Member

@edmundoa edmundoa left a comment

Other than the inline comments, the PR looks good to me.

@@ -157,11 +166,11 @@ const FieldQuickValues = React.createClass({
if (this.state.field !== undefined) {
this.setState({ loadingData: true });
if (this.state.showHistogram) {
FieldQuickValuesActions.getHistogram(this.state.field, this.state.fieldQueryObjects, this.state.options).then(() => {
FieldQuickValuesActions.getHistogram(this.state.field, this.state.fieldQueryObjects, this.state.options).finally(() => {

This comment has been minimized.

@edmundoa

edmundoa Nov 7, 2017
Member

Unfortunately this is not enough to remove the loading spinner if there was an error getting the histogram, as there will be no data at that point. As this is disabled now for ES 2.x users, I guess it's not a blocker, but still annoying if someone runs into it.

This comment has been minimized.

@edmundoa

edmundoa Nov 7, 2017
Member

I'm just wondering, why don't we use the loading state from the store in here?

Copy link
Member

@edmundoa edmundoa left a comment

As discussed offline with @bernd, the issues found here are not that big and way harder to get into them than before, so I created an issue for that and we will merge the PR.

@edmundoa edmundoa merged commit 4775212 into master Nov 7, 2017
3 of 5 checks passed
3 of 5 checks passed
@garybot2
ci-web-linter Jenkins build graylog-pr-linter-check 2048 has failed
Details
@garybot2
graylog-project/pr Jenkins build graylog-project-pr-snapshot 671 has failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details
@ghost ghost removed the ready-for-review label Nov 7, 2017
@edmundoa edmundoa deleted the issue-4289 branch Nov 7, 2017
edmundoa added a commit that referenced this pull request Nov 7, 2017
* Add elasticsearch version to cluster stats

* Reset to default options when analyzing a new message field

* Disable loading state in case of an error

If the QuickValues HTTP request fails, reset the loading state so the
user is able to change settings and try again.

Fixes #4289

* Disable stacked fields feature when running with ES < 5

(cherry picked from commit 4775212)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants