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

If elasticsearch plugin is not ready, do not block #7285

Merged
merged 2 commits into from
May 25, 2016
Merged

Conversation

ycombinator
Copy link
Contributor

Fixes #7278.

@ycombinator ycombinator added bug Fixes for quality problems that affect the customer experience blocker review P1 v5.0.0-alpha3 labels May 25, 2016
@epixa
Copy link
Contributor

epixa commented May 25, 2016

@bevacqua What do you think about this?

If the ES plugin is not ready (i.e. in red state), we won't try to use it and end
up blocking the reply.
@bevacqua
Copy link
Contributor

Doesn't make much sense to me. What is Kibana without ES?

@epixa
Copy link
Contributor

epixa commented May 25, 2016

The issue that needs to be resolved is that when Kibana is running without ES, the user should see the status page with elasticsearch marked as red. Since the config changes, requests to Kibana when ES is unavailable just hang.

@bevacqua
Copy link
Contributor

Got it. In that case this makes sense. We should set the user value to {} for consistency, anyhow.

@ycombinator
Copy link
Contributor Author

We should set the user value to {} for consistency, anyhow.

Hey @bevacqua and @epixa, thanks for the review and discussion. Please note that I actually changed the code in this PR a bit. The check for whether ES is ready or not is now done on the side of the code calling the uiSettings.getUserProvided() method, not within that method itself. I think this is cleaner because there may be scenarios when we want to legitimately wait on uiSettings.getUserProvided(). Thoughts?

@bevacqua
Copy link
Contributor

bevacqua commented May 25, 2016

@ycombinator That's the code I looked at (your latest version) -- I think we need something like this:

const red = server.plugins.elasticsearch.status.state === 'red';
payload.uiSettings.user =  red ? {} : await uiSettings.getUserProvided();

That way we don't break the assumption that uiSettings.user is an object -- even if we're not currently using it for anything other than the status page.

@ycombinator
Copy link
Contributor Author

Sounds good. Will change. Thanks.

@ycombinator
Copy link
Contributor Author

@bevacqua This is ready for a (hopefully quick and final) review. Thanks again for reviewing, especially on your day off.

@bevacqua
Copy link
Contributor

Much awesome. LGTM 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocker bug Fixes for quality problems that affect the customer experience review v5.0.0-alpha3
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants