Consolidate settings consumers for RemoteClusterService
#135722
Conversation
Moves the REMOTE_CLUSTER_SKIP_UNAVAILABLE setting into the main AffixSettings list in ClusterSettingsLinkedProjectConfigService, eliminating the need for an extra skip_unavailable specific update method on the LinkedProjectConfigListener interface, simplifying the design. Note that it is not possible to just set REMOTE_CLUSTER_SKIP_UNAVAILABLE in a settings update, the RemoteConnectionEnabled settings validator checks that the required settings for an enabled connection are also present. Resolves: ES-12860
JeremyDahlgren
added
:Distributed Coordination/Network
|
Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination) |
| remote.ensureConnected(listener.map(ignored -> RemoteClusterConnectionStatus.RECONNECTED)); | ||
| } else { | ||
| if (crossProjectEnabled == false) { | ||
| remote.setSkipUnavailable(config.skipUnavailable()); |
There was a problem hiding this comment.
To be strictly faithful to existing behaviour, I think this should be something like
| remote.setSkipUnavailable(config.skipUnavailable()); | |
| if (remote.isSkipUnavailable() != config.skipUnavailable()) { | |
| remote.setSkipUnavailable(config.skipUnavailable()); | |
| } |
| // No changes to connection configuration. | ||
| listener.onResponse(RemoteClusterConnectionStatus.UNCHANGED); |
There was a problem hiding this comment.
It is a little odd that we report UNCHANGED when skipUnavailable changes. It might be useful to add a new enum such as UPDATED for updating properties on existing connection.
| connectionMap.put(clusterAlias, remote); | ||
| remote.ensureConnected(listener.map(ignored -> RemoteClusterConnectionStatus.RECONNECTED)); | ||
| } else { | ||
| if (crossProjectEnabled == false) { |
There was a problem hiding this comment.
Is this a new change? The existing code does not seem to check this when updating it? I remember we discussed previously that skipUnavailable should not be used in CPS. But I forgot exactly what implementation we settled with. It feels that we should block setting it in the first place instead of silently ignore it?
There was a problem hiding this comment.
You are right, this would be a new check, I removed this. In #132478 we are adding the setting validator that would reject the setting being configured.
| var skipUnavailable = randomBoolean(); | ||
| applySettingsAndVerify.accept(skipUnavailable, remote1Transport); | ||
|
|
||
| // Change skip_unavailable value, but not seed node, connection should not be rebuilt, but skip_unavailable should be modified. |
There was a problem hiding this comment.
It would be great to somehow assert that the connection is the same instance, i.e. no rebuilding.
3 participants
Moves the
REMOTE_CLUSTER_SKIP_UNAVAILABLEsetting into the mainAffixSettings list inClusterSettingsLinkedProjectConfigService, eliminating the need for an extra skip_unavailable specific update method on theLinkedProjectConfigListenerinterface, simplifying the design. Note that it is not possible to just setREMOTE_CLUSTER_SKIP_UNAVAILABLEin a settings update, theRemoteClusterSettings.RemoteConnectionEnabledsettings validator checks that the required settings for an enabled connection are also present.Resolves: ES-12860