-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Add LinkedProjectConfigService
with ClusterSettings
implementation
#133834
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
Add LinkedProjectConfigService
with ClusterSettings
implementation
#133834
Conversation
a6f085b
to
acaa03d
Compare
Adds a new interface LinkedProjectConfigService for registering LinkedProjectConfigListener instances to receive linked project configuration updates. This change introduces an abstract base class and a ClusterSettings based concrete class that provides the same functionality that was previously implemented in RemoteClusterAware and RemoteClusterService. Subclasses of RemoteClusterAware have been adjusted to receive LinkedProjectConfig updates instead of Settings updates. These changes allow for other linked project config implementations to be used in the system, freeing consumers of the updates from the details of the update mechanism used. Resolves: ES-12730
acaa03d
to
616359b
Compare
Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination) |
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.
Looking great. I have mostly minor comments.
server/src/main/java/org/elasticsearch/transport/LinkedProjectConfigService.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/plugin/EqlPlugin.java
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/transport/RemoteClusterService.java
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/transport/LinkedProjectConfigService.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/transport/RemoteClusterAware.java
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/transport/RemoteClusterService.java
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/transport/RemoteClusterService.java
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/transport/ClusterSettingsLinkedProjectConfigService.java
Show resolved
Hide resolved
new ClusterSettingsLinkedProjectConfigService( | ||
settings, | ||
clusterSettings != null ? clusterSettings : ClusterSettings.createBuiltInClusterSettings(), | ||
DefaultProjectResolver.INSTANCE | ||
), |
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.
Maybe this is where we can use the LinkedProjectConfigService.NOOP
when clusterSettings == null
? I feel its behaviour is more aligned with the existing code, i.e. no settings consumer registrion and no update for remoteCluserService.
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.
Many tests going through this constructor path depend on non-noop behavior. But they do not depend on the ClusterSettings
update mechanism in all cases. I needed to refactor ClusterSettingsLinkedProjectConfigService
so that it could still be used and support getInitialLinkedProjectConfigs()
which is backed by the initial node level Settings
, without registering ClusterSettings
update consumers if the ClusterSettings
are null.
Also I moved linkedProjectConfigService.register(remoteClusterService)
call in TransportService
outside of the if (clusterSettings != null)
block. LinkedProjectConfigService
should not be coupled with ClusterSettings
.
server/src/main/java/org/elasticsearch/transport/RemoteClusterAware.java
Outdated
Show resolved
Hide resolved
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
Great work! Left two comments that I'd appreciate if you could address them. Thanks!
server/src/test/java/org/elasticsearch/transport/RemoteClusterServiceTests.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/transport/RemoteClusterAware.java
Outdated
Show resolved
Hide resolved
…lastic#133834) Adds a new interface LinkedProjectConfigService for registering LinkedProjectConfigListener instances to receive linked project configuration updates. This change introduces an abstract base class and a ClusterSettings based concrete class that provides the same functionality that was previously implemented in RemoteClusterAware and RemoteClusterService. Subclasses of RemoteClusterAware have been adjusted to receive LinkedProjectConfig updates instead of Settings updates. These changes allow for other linked project config implementations to be used in the system, freeing consumers of the updates from the details of the update mechanism used. Resolves: ES-12730
Builds on work done in #133266. Adds a new interface
LinkedProjectConfigService
for registeringLinkedProjectConfigListener
instances to receive linked project configuration updates. This change introduces an abstract base class and aClusterSettings
based concrete class that provides the same functionality that was previously implemented inRemoteClusterAware
andRemoteClusterService
.Subclasses of
RemoteClusterAware
have been adjusted to receiveLinkedProjectConfig
updates instead ofSettings
updates.These changes allow for other linked project config implementations to be used in the system, freeing consumers of the updates from the details of the update mechanism used.
Resolves: ES-12730