New per-project only settings can be defined and used by components #127252
Conversation
alexey-ivanov-es
added
>feature
:Core/Infra/Settings
|
Hi @alexey-ivanov-es, I've created a changelog YAML for you. |
| Settings.Builder builder = Settings.builder().put(knownAndValidPersistentSettings); | ||
|
|
||
| // TODO: apply only dynamic? | ||
| boolean changed = scopedSettings.updateSettings( |
There was a problem hiding this comment.
Should we update only dynamic settings here? It seems that for new projects, we can set static settings here, but for existing projects, only dynamic settings can be updated.
There was a problem hiding this comment.
I believe so. It seems updateSettings() is specifically for index settings so we should be using updateDynamicSettings() here.
elasticsearchmachine
added
the
serverless-linked
alexey-ivanov-es
removed
the
serverless-linked
|
Reopened as #127280 |
| return this; | ||
| } | ||
|
|
||
| public Settings settings() { |
There was a problem hiding this comment.
Having accessor methods on Builder seems kinda odd to me. We had to have passed this in already, so why not just hold a reference to what we pass to settings() instead?
| return this.settings; | ||
| } | ||
|
|
||
| public ProjectMetadata.Builder settings(Settings settings) { |
There was a problem hiding this comment.
nit: no need to qualify this here. We can just use Builder as the type was we do elsewhere.
| @@ -0,0 +1,5 @@ | |||
| pr: 127252 | |||
| summary: New per-project only settings can be defined and used by components | |||
There was a problem hiding this comment.
This isn't designed to be user-facing, so I wouldn't flag this as a feature for inclusion in release notes.
| * Updates transient and persistent cluster state settings if there are any changes | ||
| * due to the update. | ||
| */ | ||
| public final class SettingsUpdater { |
There was a problem hiding this comment.
It seems we might want something specific for project settings rather than re-use this class which is quite specific to cluster settings. Specifically updateProjectSettings only makes sense for project settings, and transient settings are only applicable to ClusterSettings.
I think we should introduce a separate class here for per-project settings. We can refactor shared code into an AbstractSettingsUpdator if necessary.
| return clusterState; | ||
| } | ||
|
|
||
| public synchronized ProjectMetadata updateProjectSettings( |
There was a problem hiding this comment.
We're going to want to use more granular locking here instead of marking this method synchronized. This should be scoped to the project instead of cluster wide. We can safely call this concurrently for different projects.
There was a problem hiding this comment.
I'll remove synchronized from this method when I move it to a separate class - it was there originally because updateSettings has it too. In our case, ProjectSettingsUpdater doesn't have any mutable state and is not shared between threads at all, so it doesn't make sense to keep it synchronized
No description provided.