-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Refactor RemoteClusterService to be multi-project aware #131894
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
Changes from 1 commit
4ba097b
c553912
7c65264
3615f65
0e75dde
ff1ca79
6acd58e
7d17547
8e44557
ac99eb3
5cb6d07
8264455
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -20,8 +20,11 @@ | |||||
import org.elasticsearch.action.support.RefCountingRunnable; | ||||||
import org.elasticsearch.client.internal.RemoteClusterClient; | ||||||
import org.elasticsearch.cluster.metadata.IndexNameExpressionResolver; | ||||||
import org.elasticsearch.cluster.metadata.ProjectId; | ||||||
import org.elasticsearch.cluster.node.DiscoveryNode; | ||||||
import org.elasticsearch.cluster.node.DiscoveryNodeRole; | ||||||
import org.elasticsearch.cluster.project.DefaultProjectResolver; | ||||||
import org.elasticsearch.cluster.project.ProjectResolver; | ||||||
import org.elasticsearch.common.Strings; | ||||||
import org.elasticsearch.common.settings.ClusterSettings; | ||||||
import org.elasticsearch.common.settings.SecureSetting; | ||||||
|
@@ -31,6 +34,7 @@ | |||||
import org.elasticsearch.common.util.concurrent.ConcurrentCollections; | ||||||
import org.elasticsearch.common.util.concurrent.CountDown; | ||||||
import org.elasticsearch.common.util.concurrent.EsExecutors; | ||||||
import org.elasticsearch.core.FixForMultiProject; | ||||||
import org.elasticsearch.core.IOUtils; | ||||||
import org.elasticsearch.core.TimeValue; | ||||||
import org.elasticsearch.indices.IndicesExpressionGrouper; | ||||||
|
@@ -53,6 +57,7 @@ | |||||
import java.util.function.BiFunction; | ||||||
import java.util.function.Function; | ||||||
import java.util.function.Supplier; | ||||||
import java.util.stream.Collectors; | ||||||
import java.util.stream.Stream; | ||||||
|
||||||
import static org.elasticsearch.common.settings.Setting.boolSetting; | ||||||
|
@@ -155,9 +160,11 @@ public boolean isRemoteClusterServerEnabled() { | |||||
} | ||||||
|
||||||
private final TransportService transportService; | ||||||
private final Map<String, RemoteClusterConnection> remoteClusters = ConcurrentCollections.newConcurrentMap(); | ||||||
private final Map<ProjectId, Map<String, RemoteClusterConnection>> remoteClusters = ConcurrentCollections.newConcurrentMap(); | ||||||
private final RemoteClusterCredentialsManager remoteClusterCredentialsManager; | ||||||
private final ProjectResolver projectResolver; | ||||||
|
||||||
@FixForMultiProject(description = "Inject the ProjectResolver instance.") | ||||||
RemoteClusterService(Settings settings, TransportService transportService) { | ||||||
super(settings); | ||||||
this.enabled = DiscoveryNode.isRemoteClusterClient(settings); | ||||||
|
@@ -167,6 +174,21 @@ public boolean isRemoteClusterServerEnabled() { | |||||
if (remoteClusterServerEnabled) { | ||||||
registerRemoteClusterHandshakeRequestHandler(transportService); | ||||||
} | ||||||
this.projectResolver = DefaultProjectResolver.INSTANCE; | ||||||
} | ||||||
|
||||||
/** | ||||||
* Returns the map of connections for the {@link ProjectId} currently returned by the {@link ProjectResolver}. | ||||||
*/ | ||||||
private Map<String, RemoteClusterConnection> getConnectionsMap() { | ||||||
return getConnectionsMap(projectResolver.getProjectId()); | ||||||
} | ||||||
JeremyDahlgren marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||
|
||||||
/** | ||||||
* Returns the map of connections for the given {@link ProjectId}. | ||||||
*/ | ||||||
private Map<String, RemoteClusterConnection> getConnectionsMap(ProjectId projectId) { | ||||||
JeremyDahlgren marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||
return remoteClusters.computeIfAbsent(projectId, unused -> ConcurrentCollections.newConcurrentMap()); | ||||||
|
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.
JeremyDahlgren marked this conversation as resolved.
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.
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.
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.
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.
JeremyDahlgren marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
JeremyDahlgren marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
JeremyDahlgren marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
Outdated
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.
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.
JeremyDahlgren marked this conversation as resolved.
Show resolved
Hide resolved
JeremyDahlgren marked this conversation as resolved.
Show resolved
Hide resolved
Outdated
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.
Not sure if async search would provide the necessary project context.
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.
I added the fix annotation here as well.
Uh oh!
There was an error while loading. Please reload this page.