[WIP] ES-11463 Components are notified to updates to per-project settings

[alexey-ivanov-es]

This commit introduces a notification system for components when per-project settings are updated. The implementation refactors the existing settings framework by introducing settings context concept to AbstractScopedSettings. In order to maintain backward-compatibility with existing code it also introduces AbstractContextlessScopedSettings which becomes ancestor for IndexScopedSetting and ClusterSettings.

It also adds additional logic to ClusterApplierService.applyChanges - when cluster state changes occur, the system compares old and new project settings for each known project and triggers settings updates for any changes detected.

[cla-checker-service]

💚 CLA has been signed

[alexey-ivanov-es]

Should we have project id here?

[alexey-ivanov-es]

I don't like this name, but didn't manage to find anything better

[tvernum]

Can we add Javadoc then? Since the name isn't especially informative.

[alexey-ivanov-es]

There are still discussions about the design of ProjectScopedSettings and its interoperability with ClusterSettings, so I've marked it as WIP

[elasticsearchmachine]

Pinging @elastic/es-core-infra (Team:Core/Infra)

[ywangd]

Thanks for working on this!

I am not entirely sure that I like how addSettingsUpdateConsumer methods work in the new type system. The base versions are defined with BiConsumer which are basically ignored by the contextless subtypes. But both versions are visible to the contextless subtypes and resulting 2x number of methods. I wonder, is it necessary to know about the context (ProjectId) when registering an update consumer? I cannot think of a great reason that a setting update is only of interest for some projects but not others. So I'd imagine when we call ProjectScopedSettings#addSettingsUpdateConsumer, we are registering it for all projects (existing and future).

The current proposal has ProjectScopedSettings internally manges settings for all projects. That works for me. The other option could be one such object for each project similar to IndexScopedSettings. I am not recommending this approach. But would like to add that the context (ProjectId) is not necessary with this approach either since each ProjectScopedSettings already identifies the project.

On the consumer side, we do need the context (ProjectId). As discussed in other places, we could achieve this without method signature changes by using ThreadContext and ProjectResolver? Since applyChanges is itself executed without any project context (necessary since we don't allow changing project context if there is one already), it should be possible to wrap the projectScopedSettings.applySettings(projectId, newProjectSettings) call with project context?

I am probably not updated with all the discussions around this topic. So there maybe strong reasons for the current proposal. I appreciate the progress here regardless of which choice we end up taking. 🙏

[tvernum]

I'm a little unsure where we're heading here. There's some strangeness (as @ywangd touched on) but I don't know whether it's intended to be a temporary state on the way to something else, or an attempt to move in a different than we've had in the past, or something else.

To be fair, what we started with was also a bit strange - we've been putting off a bunch of Settings cleanup work for some time.

Things that were already a bit weird (please correct me if I've made a mistake here, but the weirdness makes it hard to keep track of how everything works, or is supposed to work)

• We have a common base class for both cluster settings (which are effectively global) and index settings (which are not). But that base class can't really decide if it wants to be global or not. It manages the set of Setting instances that are valid in this scope (which is global) but also has the current Settings (which are per-target instance). That is, it feels like it's doing 2 jobs.
• Because it's doing 2 jobs, some of the methods are hard to reason about. When you register an updateConsumer are you going to get updates for every instance or one instance?
• IndexScopedSettings's response to having 2 jobs is to embrace the weirdness. We have some instances that are trying to be global (created with the IndexScopedSettings(Settings, Set<Setting<?>>) constructor) and some instances that are children of a global object (created via copy() and the IndexScopedSettings(Settings, IndexScopedSettings, IndexMetadata) constructor).
• Which means you can register an update-consumer on that parent object, and it will be copied to each child. But only if you do it early in the node lifecycle - the listeners are just copied to new children, they're not actually shared.
• But, we don't have just a singleton for that global object... No, we have one in SettingsModule (indexScopedSettings) and in IndexSettings (via IndexSettingsModule.configure()), and also IndexScopedSettings.DEFAULT_SCOPED_SETTINGS. And some other methods that can create a non-child instance of IndexScopedSettings and are in production code, but only seem to be used by tests.
• Yet, despite this, a index-specific instance of IndexScopedSettings doesn't track which index it's for. The IndexMetadata is passed into the constructor, but we only use that to get the current applied settings on the index, and create a scoped logger. So if you get a callback on your updateConsumer you can't tell which index it's for. So it's probably best not to register a consumer on the parent object, because the callbacks are almost useless.
• And, effectively, the "per instance" behaviour of IndexScopedSettings is duplicated by IndexSettings. IndexSettings relies on IndexScopedSettings to track which settings are Index scoped, and notify consumers, but the "this index has these settings" part is managed with IndexSettings.

Now we have another type of per-instance setting: Project

This PR takes a different approach to handling that than the one IndexScopedSettings does. That's probably a good idea, given the issues above, but it does feel like we're trying to tack another way of handling this on top of class structure that doesn't really work. I can't tell if we have a plan to somehow bring this all back together neatly, or if we're just going to end up with 2 different types of weirdness.

I think the root of the problem is that AbstractScopedSettings is doing 3 things that don't actually work well in the same class

1. Tracking Setting instances that apply in this scope, and validating setting updates against those
2. Tracking and notifying listeners
3. Tracking the current settings state.

If we split 1 and 3 into different classes, then I think things would be a bit simpler.

• The XyzScopedSettingsService would know which settings are allowed in Xyz scope, and you could pass a Settings object to it to determine whether it's valid.
• The XyzScopedSettingState would hold the current (and previous, I suppose) settings for a particular instanceof Xyz.
• The XyzScopedSettingsService could hold a collection of XyzScopedSettingState keyed by K (a ProjectId or Index). In the case of ClusterScopedSettingsService we only need a single value, but we could logically key it by ClusterName or UUID.
• We would have to decide where the update consumers live. I can see any of the options below working, but in practice, I think most consumers want to get notified about changes on any target (project/index), so attaching the consumer to XyzScopedSettingsService seems to be a better fit.
  • It's on XyzScopedSettingsService and the consumer takes 2 arguments (K, Settings)
  • It's on XyzScopedSettingsService and the consumer takes 1 argument (XyzScopedSettingsState), which which the K and Settings can be derived
  • It's on XyzScopedSettingsState and the consumer takes 2 arguments (XyzScopedSettingsState, Settings)
  • It's on XyzScopedSettingsState and the consumer takes 1 arguments (Settings) and the caller is supposed to keep track of which State it applied it to.

But, maybe you have a different plan. Do we have a vision for how we want this to look when we're done? Is this just step 1?

[alexey-ivanov-es]

So I'd imagine when we call ProjectScopedSettings#addSettingsUpdateConsumer, we are registering it for all projects (existing and future).

Yes

it should be possible to wrap the projectScopedSettings.applySettings(projectId, newProjectSettings) call with project context?

Yes, this should be possible.

I am not entirely sure that I like how addSettingsUpdateConsumer methods work in the new type system. The base versions are defined with BiConsumer which are basically ignored by the contextless subtypes. But both versions are visible to the contextless subtypes and resulting 2x number of methods

I agree that it would be better not to expose methods with context parameter in AbstractContextlessScopedSettings, however, there are examples (CCS/CPS) where the same code should be able to work with both ProjectScopedSettings/ClusterSettings. As you mentioned, there is another option - pass ProjectId to consumers implicitly and expect that they would get it via ProjectResolver. In this scenario, every project setting update consumer must use ProjectResolver to get the project id for the update and this is not obvious. And I'm not sure that it is a good idea, to force everyone to write code getting the project id considering that this parameter is mandatory. I am not sure which option is worse.

The other option could be one such object for each project similar to IndexScopedSettings. I am not recommending this approach. But would like to add that the context (ProjectId) is not necessary with this approach either since each ProjectScopedSettings already identifies the project.

We were considering this approach, but despite looking similar to what we want to achieve with projects, the approach, as you rightly mentioned, has a lot of problems, so we decided to do project settings differently.

Do we have a vision for how we want this to look when we're done? Is this just step 1?

I don't think we have a clear end-to-end vision for settings as a whole right now. There are some improvement ideas, but nothing comprehensive that would clean up the whole system. This PR definitely adds another layer of complexity, but a full redesign would be a huge effort and isn't something we aimed to solve here.

[mark-vieira]

This PR takes a different approach to handling that than the one IndexScopedSettings does. That's probably a good idea, given the issues above, but it does feel like we're trying to tack another way of handling this on top of class structure that doesn't really work.

I definitely agree. I think the issue here is on the consumer side. For indices the things that listen to updates tend to already be per-index, so it makes sense to have things scoped all the way down. For multi-project however, we don't intend to have a lot of per-project components (or at least that's our aim), so following the pattern of IndexScopedSettings doesn't reallly work, we don't want to have a bunch of different ProjectScopedSettings.

As you mentioned, there is another option - pass ProjectId to consumers implicitly and expect that they would get it via ProjectResolver. In this scenario, every project setting update consumer must use ProjectResolver to get the project id for the update and this is not obvious. And I'm not sure that it is a good idea, to force everyone to write code getting the project id considering that this parameter is mandatory. I am not sure which option is worse.

I'm not sure this is avoidable. Components that leverage per-project settings have to be aware of this already. By definition, if they care that the setting is per-project, they are already doing this bookeeping internally. I don't think performing the extra step of "oh, which project is this for" is that big a deal.

3. Tracking the current settings state.

Only IndexScopedSettings does this though, right? ClusterSettings doesn't actually hold on to a Settings from what I can tell, which makes sense, those are global, so it doesn't make sense to keep a copy of them, where as IndexScopedSettings is per-index, so it does have to keep that state.

I think the distinction of "context aware" vs "per-instance" may not be the right one but it's the one we got. It seems (at least in the short term) that the main contention is on how we pass that context. I tend to agree that applying even more complexity on top of the already fragmented settings infra is probably not what we want to do. As @alexey-ivanov-es states, a larger refactoring of all this stuff isn't really on the table at the moment.

I'd propose ditching AbstractContextlessScopedSettings and going the implicit context route mentioned. That is, just grabbing the ProjectId from ThreadContext via a ProjectResolver. It's perhaps a bit more work on the consumer end, but I think we already have a well established way of having components as "what project is this for?" whereas we don't have an agreed on cohesive pattern for all this settings stuff.

It sort of then begs the question of whether a ProjectScopedSettings is even required? What exactly would it do vs ClusterSettings? Is there a reason we'd have to separate the two at all? In my mind, the settings are either index-scoped, or they aren't. In which case they are effectively "cluster settings" and then we just ask which project they apply to. Could we eliminate all this nonsense, just continue to have folks register updates as they do, and just handle all the per-project stuff implicitly through ThreadContext?