-
Notifications
You must be signed in to change notification settings - Fork 25.3k
Make IlmHealthIndicatorService
work in multi-project cluster
#129961
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
Conversation
This does _not_ make the health indicator project-aware, it merely avoids exceptions in case there are multiple projects in the cluster. The health indicator would require significant refactoring to be made project-aware, which is not worth it since ILM will not be running in a multi-project context (i.e. serverless).
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.
Should we add a NotMultiProjectCapable
annotation to this class once that is merged?
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.
Just one comment. Thanks.
* This method solely exists because we are not making ILM properly project-aware and it's not worth the investment of altering this | ||
* health indicator to be project-aware. | ||
*/ | ||
private static ProjectMetadata getDefaultILMProject(ClusterState state) { |
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.
Is this going to get your new 'not MP compatible' annotation?
I assume that adding an assertion runs into the same problems that we have in the other cases?
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.
Yeah, I just added the annotation.
I assume that adding an assertion runs into the same problems that we have in the other cases?
Adding the assertion would lead to similar problems. Since this is not a cluster state listener, the impact is far less, but we still call the health report here. Even though it's just one test/test suite, I'm still not very comfortable with the idea of trying to modify the big XPackRestIT
or this test (we could retrieve a specific indicator rather than the full health report). The former is a larger effort and the latter reduces test coverage IMO.
…ic#129961) This does _not_ make the health indicator project-aware, it merely avoids exceptions in case there are multiple projects in the cluster. The health indicator would require significant refactoring to be made project-aware, which is not worth it since ILM will not be running in a multi-project context (i.e. serverless).
This does not make the health indicator project-aware, it merely avoids exceptions in case there are multiple projects in the cluster. The health indicator would require significant refactoring to be made project-aware, which is not worth it since ILM will not be running in a multi-project context (i.e. serverless).