-
Notifications
You must be signed in to change notification settings - Fork 25.1k
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
Documenting master_is_stable health API settings #87901
Conversation
Pinging @elastic/es-docs (Team:Docs) |
Pinging @elastic/es-data-management (Team:Data Management) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Keith. Left some comments
++++ | ||
|
||
The following are the _expert-level_ settings available for configuring the | ||
<<health-api, Health API>>. It is not recommended to change any of these |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say this configures an internal diagnostics service as opposed to the API.
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I should change it to "for configuring an internal diagnostics service that is exposed through the Health API"? To the user changes to these settings will only be visible via the health api right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this idea. Essentially these are internal diagnostics, they are now used in the health API but they might have more use cases, right?
What about calling this page "Health diagnostic settings" or "Internal health diagnostic settings" and only refer to the health API in the begging. Something like: "the outcome of this service is exposed via the Health API"?
I think this could open up different ways of phrasing the impact of the settings. For example:
This many changes or more will be reported as a problem in the Health API
could be phrased as This many changes or more will be reported as unhealthy
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
++ I like it too and I like your suggestions @gmarouli !
==== Cluster level settings | ||
|
||
`health.master_history.has_master_lookup_timeframe`:: | ||
(<<static-cluster-setting,Static>>) The amount of time the API looks back to see if the cluster has had |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say we're looking back to see if an instance observed any masters. We're not making cluster-level decisions here.
What do you think?
|
||
`master_history.max_age`:: | ||
(<<static-cluster-setting,Static>>) The maximum amount of time that the master history is kept in memory | ||
for use in the Health API. Master node changes older than this time will not be considered in the Health |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for use in the Health API. Master node changes older than this time will not be considered in the Health | |
. Master node changes older than this time will not be considered in the Health |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should potentially decouple these settings from the health API (I see it's mentioned again)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we don't say how these impact the end user, then no one would ever change them, right? That might not be such a bad thing though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work @masseyke , thank you for setting this page up, I will need soon too.
My only comment is in the existing discussion https://github.com/elastic/elasticsearch/pull/87901/files#r905907682.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with a few suggestions
Thanks Keith
Co-authored-by: Andrei Dan <andrei.dan@elastic.co>
Co-authored-by: Andrei Dan <andrei.dan@elastic.co>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for iterating on this Keith
This adds documentation for the settings used in the master_is_stable part of the health API.
Preview URL: https://elasticsearch_87901.docs-preview.app.elstc.co/guide/en/elasticsearch/reference/master/health-api-settings.html