Move per-project settings out of ProjectMetadata #129068
Conversation
alexey-ivanov-es
added
>enhancement
:Core/Infra/Settings
elasticsearchmachine
added
serverless-linked
|
Hi @alexey-ivanov-es, I've created a changelog YAML for you. |
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
| @@ -285,6 +285,7 @@ static TransportVersion def(int id) { | |||
| public static final TransportVersion ILM_ADD_SKIP_SETTING = def(9_089_0_00); | |||
| public static final TransportVersion ML_INFERENCE_MISTRAL_CHAT_COMPLETION_ADDED = def(9_090_0_00); | |||
| public static final TransportVersion IDP_CUSTOM_SAML_ATTRIBUTES_ALLOW_LIST = def(9_091_0_00); | |||
| public static final TransportVersion CLUSTER_STATE_PROJECTS_SETTINGS = def(9_092_0_00); | |||
There was a problem hiding this comment.
Do we actually need to introduce a new transport version here? The per-project stuff was never included in a stack release, and even in serverless it's behind the multi-project flag which isn't enabled in any production environment. I believe it's safe to remove this w/o introducing a new transport version.
There was a problem hiding this comment.
@rjernst is my logic sound here?
There was a problem hiding this comment.
Maybe. I think the problem is when the feature flag is removed, a transport version would need to be introduced. Adding it here seems safer.
| @@ -846,6 +855,8 @@ private static BytesReference sortRoutingTableXContent(String jsonContent) throw | |||
| } | |||
|
|
|||
| private static ClusterState buildMultiProjectClusterState(DiscoveryNode... nodes) { | |||
| ProjectId projectId1 = ProjectId.fromId("3LftaL7hgfXAsF60Gm6jcD"); | |||
There was a problem hiding this comment.
We can use randomProjectIdOrDefault() or randomUniqueProjectId() here instead of hard-coding values.
There was a problem hiding this comment.
This project-id is used in the serialized form of cluster state - using a random id would require generating the expected string, and I don't think it's worth it
| import java.util.Iterator; | ||
| import java.util.Map; | ||
|
|
||
| public class ProjectsStateRegistry extends AbstractNamedDiffable<ClusterState.Custom> implements ClusterState.Custom { |
There was a problem hiding this comment.
nit: Can we make this ProjectStateRegistry. I think the "registry" bit already implies this is a collection, no need to make it plural.
| /** | ||
| * Returns a new instance of ProjectsStateRegistry containing all projects from the cluster state and settings for the specified project | ||
| */ | ||
| public static ProjectsStateRegistry setProjectSettings(ClusterState clusterState, ProjectId projectId, Settings settings) { |
There was a problem hiding this comment.
This method seems unnecessary and confusing. The naming implies this mutates the given ClusterState but that's not the case. I'd just remove this and use the builder directly. It's more clear as to what his happening here.
| projectsSettings = in.readMap(ProjectId::readFrom, Settings::readSettingsFromStream); | ||
| } | ||
|
|
||
| public ProjectsStateRegistry(Map<ProjectId, Settings> projectsSettings) { |
There was a problem hiding this comment.
I'm inclined to make this constructor private. Folks using this class should use the builder instead, and we know we plan to add more items to this type, so having this public just means there will be more refactoring needed when that happens as we'll have to add new arguments here.
| this.projectsSettings = projectsSettings; | ||
| } | ||
|
|
||
| public static Settings getProjectSettings(ProjectId projectId, ClusterState clusterState) { |
There was a problem hiding this comment.
I'm not sure if we want this convenience method either. We should encourage folks to use ProjectState to access stuff like this and having multiple methods to do the same thing muddies that convention.
To better support project restoration after deletion, this change moves project Settings from
ProjectMetadatato the new custom in theClusterState. It also introduces a new transport version for cluster state serialization. Reserved cluster state for project settings remains withinProjectMetadata.Note: In mixed-version multiproject clusters, this may cause existing settings for projects to temporarily disappear until all nodes have been upgraded and restarted.