Refactor RemoteClusterService to be multi-project aware #131894

JeremyDahlgren · 2025-07-25T01:36:53Z

This is the first in a series of PRs to refactor the RemoteClusterService to support multi-project. This first PR uses the DefaultProjectResolver.INSTANCE in the constructor. A follow up PR will handle refactoring needed to pass in the ProjectResolver in the constructor. Other follow up PRs will handle refactoring RemoteClusterAware.updateRemoteCluster() and other settings related methods that require a project ID to be provided.

Relates: ES-11576

This is the first in a series of PRs to refactor the RemoteClusterService to support multi-project. This first PR uses the DefaultProjectResolver.INSTANCE in the constructor. A follow up PR will handle refactoring needed to pass inthe ProjectResolver in the constructor. Other follow up PRs will handle refactoring RemoteClusterAware.updateRemoteCluster() and other settings related methods that require a project ID to be provided.

ywangd

Thanks for working on this. I left some comments.

server/src/main/java/org/elasticsearch/transport/RemoteClusterService.java

ywangd · 2025-07-25T02:38:19Z

server/src/main/java/org/elasticsearch/transport/RemoteClusterService.java

+     * Returns the map of connections for the given {@link ProjectId}.
+     */
+    private Map<String, RemoteClusterConnection> getConnectionsMap(ProjectId projectId) {
+        return remoteClusters.computeIfAbsent(projectId, unused -> ConcurrentCollections.newConcurrentMap());


If we go with my previous suggestion, this needs to change as well since immutable map does not support computeIfAbsent. So something like the following would be necessary

Suggested change

return remoteClusters.computeIfAbsent(projectId, unused -> ConcurrentCollections.newConcurrentMap());

return projectResolver.supportsMultipleProjects() ? remoteClusters.computeIfAbsent(projectId, unused -> ConcurrentCollections.newConcurrentMap()) : remoteClusters.get(projectId);

We can also assert projectId is default when projectResolver.supportsMultipleProjects() == false and vice versa.

server/src/main/java/org/elasticsearch/transport/RemoteClusterService.java

ywangd · 2025-07-25T03:03:20Z

server/src/main/java/org/elasticsearch/transport/RemoteClusterService.java

    public Set<String> getRegisteredRemoteClusterNames() {
-        return remoteClusters.keySet();
+        return getConnectionsMap().keySet();
    }


Methods like this one relies on the project context configured correctly by the caller. Not an issue for non-MP. But we will need to double check for MP. Probably worth to log an issue so that we don't forget.

This method is used by TransportClusterStatsAction which can be argued as cluster scoped. In that case, we'd need loop through all projects for this method. This will be a big change since it affects REST API output. So I think we can go with this now and revisit it later similar to how we did for indices stats and add a fix annotation there.

I added the fix annotation here and in a few other places. It might be good to review the package-access test methods as well as some of the other public methods that use the connections map and see if we can reduce the overall API surface area. It would help with reasoning about all the multi-project use cases. I can submit small separate PRs if I find some scenarios where we'd get some benefit from refactoring.

ywangd · 2025-07-25T03:12:42Z

server/src/main/java/org/elasticsearch/transport/RemoteClusterService.java

+    @FixForMultiProject(description = "Refactor to provide the project ID that the linked alias and settings are associated with.")
    @Override
    protected void updateRemoteCluster(String clusterAlias, Settings settings) {
+        final var projectId = projectResolver.getProjectId();


This is called by settings applier which does not have project context today so that resolver won't work for MP. Can we link an issue (if there is one for settings changes) here to remind us? Maybe the fix annotation is for this purpose? If so, I'd prefer we move it directly above projectId assignment line.

server/src/main/java/org/elasticsearch/transport/RemoteClusterService.java

ywangd · 2025-07-25T03:17:04Z

server/src/main/java/org/elasticsearch/transport/RemoteClusterService.java

                "this node does not have the " + DiscoveryNodeRole.REMOTE_CLUSTER_CLIENT_ROLE.roleName() + " role"
            );
        }
+        final var projectConnectionsMap = getConnectionsMap();


Not sure if async search would provide the necessary project context.

I added the fix annotation here as well.

server/src/main/java/org/elasticsearch/transport/RemoteClusterService.java

elasticsearchmachine · 2025-07-25T21:22:20Z

Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination)

ywangd

I have one comment about sticking to one way of handling ProjectId for public methods.

server/src/main/java/org/elasticsearch/transport/RemoteClusterService.java

ywangd · 2025-07-28T04:03:57Z

server/src/main/java/org/elasticsearch/transport/RemoteClusterService.java

+        @FixForMultiProject(description = "ES-12270: Add a parameter for the project ID associated with the linked alias and settings.")
+        final var projectId = projectResolver.getProjectId();


I think we are not entirely sure whether the projectId will be passed-in explicitly or resolved from context since this is still being discussed over #131637

I adjusted the annotation descriptions so we refactor if needed. It is sounding like the preference will be to try to use the project resolver, per Mark's comment in #131637.

ywangd · 2025-07-28T04:21:57Z

server/src/main/java/org/elasticsearch/transport/RemoteClusterService.java

+        @FixForMultiProject(description = "Refactor method parameters to supply the project ID.")
+        final var projectId = projectResolver.getProjectId();


It is ideal if all public methods (except those targeting all projects) of this class have a consistent way for handling ProjectId, either:

Take a ProjectId parameter

Or, resolving from threadContext with projectResolver

Otherwise it's rather difficult to reason with the behaviour. Which one is your preference? My current thinking is that we should go with the 2nd option [0] (private methods can sure and will likely need to take ProjectId parameter). Right now we have a mixed usages across all public methods depending wether it ends up calling getConnectionsMapForCurrentProject or getConnectionsMapForProject. I'd prefer we tighten them up.

[0] An outstanding issue is whether project settings update are performed with context, i.e. depending on the outcome of #131637. It's possible that we will go with the threadContext path. Even if we don't, it's reasonable to make those as exception since they are mutating methods while other reading methods should stick to one convention.

I updated the annotations and changed one method where we could call getConnectionsMapForCurrentProject() instead of getConnectionsMapForProject().

The only places outside of getConnectionsMapForCurrentProject() where the project resolver is used to get the project ID is where we need it ahead of time for logging. So right now it should be consistent that the current plan is to obtain the project ID via the resolver. There are a few fix annotations for methods where we would need to revisit use cases to determine if cluster-wide versions need to be supported, or if supporting context would not be available for use in the resolver.

This change eliminates or moves the following methods from RemoteClusterService: isCrossClusterSearchEnabled() isRemoteNodeConnected() isRemoteClusterRegistered() This is part of an effort to reduce the API surface area of RemoteClusterService. Relates: elastic#131894, elastic#131948

…ments

This change eliminates or moves the following methods from RemoteClusterService: isCrossClusterSearchEnabled() isRemoteNodeConnected() isRemoteClusterRegistered() This is part of an effort to reduce the API surface area of RemoteClusterService. Relates: #131894, #131948

ywangd

LGTM

ywangd · 2025-07-31T05:12:07Z

server/src/main/java/org/elasticsearch/transport/RemoteClusterService.java

-    /**
-     * This method updates the list of remote clusters. It's intended to be used as an update consumer on the settings infrastructure
-     *
-     * @param clusterAlias a cluster alias to discovery node mapping representing the remote clusters seeds nodes
-     * @param newSettings the updated settings for the remote connection
-     * @param listener a listener invoked once every configured cluster has been connected to
-     */


Can we salvage javadocs for the parameters to the overloaded method below it, i.e. this method

private synchronized void updateRemoteCluster( ProjectId projectId, String clusterAlias, Settings newSettings, boolean forceRebuild, ActionListener<RemoteClusterConnectionStatus> listener )

?

ywangd · 2025-07-31T05:17:16Z

server/src/main/java/org/elasticsearch/transport/RemoteClusterService.java

+    @FixForMultiProject(description = "Refactor to supply the project ID associated with the alias and settings, or eliminate this method.")
    void updateRemoteCluster(String clusterAlias, Settings newSettings, ActionListener<RemoteClusterConnectionStatus> listener) {


IIUC, this method only has test usages after this PR. So yeah I'd prefer to remove it eventually.

server/src/main/java/org/elasticsearch/transport/RemoteClusterService.java

…132037) This change eliminates or moves the following methods from RemoteClusterService: isCrossClusterSearchEnabled() isRemoteNodeConnected() isRemoteClusterRegistered() This is part of an effort to reduce the API surface area of RemoteClusterService. Relates: elastic#131894, elastic#131948

This is the first in a series of PRs to refactor the RemoteClusterService to support multi-project. This first PR uses the DefaultProjectResolver.INSTANCE in the constructor. A follow up PR will handle refactoring needed to pass in the ProjectResolver in the constructor. Other follow up PRs will handle refactoring RemoteClusterAware.updateRemoteCluster() and other settings related methods that require a project ID to be provided.

* upstream/main: (822 commits) Improve Semantic Text Exists Query Tests (elastic#132283) Make hierarchical k-means over centroids cheaper (elastic#132316) Remove unnecessary listener.delegateFailure in IndexShard#ensureMutable (elastic#132294) Add missing release note (elastic#132319) Unmute elastic#131803 (elastic#132295) Include bytes for live docs in ShardFieldStats (elastic#132232) Fix default missing index sort value of data_nanos pre 7.14 (elastic#132162) [DiskBBQ] Quantize centroids using 7 bits instead of 4 bits (elastic#132261) Use panamized version for windows in Int7VectorScorer (elastic#132311) Mute org.elasticsearch.xpack.ml.integration.AutodetectMemoryLimitIT testTooManyByAndOverFields elastic#132310 Mute org.elasticsearch.xpack.ml.integration.AutodetectMemoryLimitIT testManyDistinctOverFields elastic#132308 Update 8.17 version to 8.17.10 (elastic#132303) Mute org.elasticsearch.datastreams.DataStreamsClientYamlTestSuiteIT test {p0=data_stream/10_basic/Create hidden data stream with match all template} elastic#132298 Add random queries to logsdb data generation tests (elastic#132109) ES|QL Inference runner refactoring (elastic#131986) Add basic example to linear-retriever.md (elastic#132196) Refactor RemoteClusterService to be multi-project aware (elastic#131894) ESQL: Mark csv-spec tests (elastic#132098) Mute org.elasticsearch.common.logging.JULBridgeTests testThrowable elastic#132280 Bump versions after 8.19.0 release ...

Follow up to work done in elastic#131894 to make RemoteClusterService multi-project aware. This task plumbs the ProjectResolver down into the RemoteClusterService from NodeConstruction. Resolves: ES-12572

Follow up to work done in #131894 to make RemoteClusterService multi-project aware. This task plumbs the ProjectResolver down into the RemoteClusterService from NodeConstruction. Resolves: ES-12572

JeremyDahlgren requested a review from ywangd July 25, 2025 01:36

JeremyDahlgren added :Distributed Coordination/Network Http and internode communication implementations >refactoring Team:Distributed Coordination Meta label for Distributed Coordination team v9.2.0 labels Jul 25, 2025

ywangd reviewed Jul 25, 2025

View reviewed changes

Address code review comments

c553912

JeremyDahlgren marked this pull request as ready for review July 25, 2025 21:21

JeremyDahlgren mentioned this pull request Jul 25, 2025

Remove RemoteClusterService.getConnections() method #131948

Merged

ywangd reviewed Jul 28, 2025

View reviewed changes

Move new private methods closer to bottom of class

7c65264

JeremyDahlgren mentioned this pull request Jul 28, 2025

Relocate RemoteClusterService methods only used in tests. #132037

Merged

JeremyDahlgren added 5 commits July 28, 2025 15:31

Adjust annotation descriptions and ProjectResolver use per review com…

3615f65

…ments

Merge branch 'main' into es-11576-multi-project-aware

0e75dde

Merge branch 'main' into es-11576-multi-project-aware

ff1ca79

Merge branch 'main' into es-11576-multi-project-aware

6acd58e

Merge branch 'main' into es-11576-multi-project-aware

7d17547

Merge branch 'main' into es-11576-multi-project-aware

8e44557

ywangd approved these changes Jul 31, 2025

View reviewed changes

JeremyDahlgren added 3 commits July 31, 2025 08:54

Change "project ID" to just "project" in log messages

ac99eb3

Salvage javadoc for main updateRemoteCluster() method

5cb6d07

Merge branch 'main' into es-11576-multi-project-aware

8264455

JeremyDahlgren merged commit 4be260a into elastic:main Jul 31, 2025
33 checks passed

JeremyDahlgren mentioned this pull request Aug 15, 2025

Add ProjectResolver to RemoteClusterService constructor #133006

Merged

	return remoteClusters.computeIfAbsent(projectId, unused -> ConcurrentCollections.newConcurrentMap());
	return projectResolver.supportsMultipleProjects() ? remoteClusters.computeIfAbsent(projectId, unused -> ConcurrentCollections.newConcurrentMap()) : remoteClusters.get(projectId);

		@FixForMultiProject(description = "ES-12270: Add a parameter for the project ID associated with the linked alias and settings.")
		final var projectId = projectResolver.getProjectId();

		@FixForMultiProject(description = "Refactor method parameters to supply the project ID.")
		final var projectId = projectResolver.getProjectId();

		@FixForMultiProject(description = "Refactor to supply the project ID associated with the alias and settings, or eliminate this method.")
		void updateRemoteCluster(String clusterAlias, Settings newSettings, ActionListener<RemoteClusterConnectionStatus> listener) {

Refactor RemoteClusterService to be multi-project aware #131894

Refactor RemoteClusterService to be multi-project aware #131894

Uh oh!

Conversation

JeremyDahlgren commented Jul 25, 2025

Uh oh!

ywangd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

elasticsearchmachine commented Jul 25, 2025

Uh oh!

ywangd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JeremyDahlgren Jul 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ywangd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

JeremyDahlgren Jul 28, 2025 •

edited

Loading