-
Couldn't load subscription status.
- Fork 25.6k
Refactor some ILM tests to not use deprecated methods #131417
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
And updates `MetadataMigrateToDataTiersRoutingService` to accept a `ProjectState` instead of a `ProjectResolver`.
|
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.
LGTM as far as the refactoring goes, but I do have a question around what this code is doing.
| ); | ||
| return Tuple.tuple( | ||
| ClusterState.builder(currentState).metadata(mb).putProjectMetadata(newProjectMetadataBuilder).build(), | ||
| ClusterState.builder(currentState.cluster()).metadata(mb).putProjectMetadata(newProjectMetadataBuilder).build(), |
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.
Your change here seems like a straight refactoring, so that's fine, but I have a wider question. This code is working at project scope, but it's also mutating state at the cluster scope (removing the ENFORCE_DEFAULT_TIER_PREFERENCE setting). That can't be safe, can it? Do we just not care because it's ILM? If so, it feels like we should use your annotation to call that out. If we plan to make it safe, is that tracked somewhere?
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 agree that it's not ideal, but I figured we can make an exception for ILM. I have a feeling that we'll be making some changes to ProjectState at some point, to avoid these kinds of things (i.e. to make it safer in terms of what is modifiable). When we do so, we'll have to take places like this into account. Adding a @NotMultiProjectCapable sounds like a good idea. Will do.
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.
|
Nit: I just noticed you applied the |
| ); | ||
| return Tuple.tuple( | ||
| ClusterState.builder(currentState).metadata(mb).putProjectMetadata(newProjectMetadataBuilder).build(), | ||
| ClusterState.builder(currentState.cluster()).metadata(mb).putProjectMetadata(newProjectMetadataBuilder).build(), |
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.
|
@elasticmachine update branch |
And updates
MetadataMigrateToDataTiersRoutingServiceto accept aProjectStateinstead of aProjectResolver.