-
Notifications
You must be signed in to change notification settings - Fork 25.5k
Consolidate settings consumers for RemoteClusterService
#135722
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 2 commits
5251826
23d3ec6
be18795
ae572d6
79ebe33
71646cd
69a272c
491f669
77400bc
80d0d0e
1650973
13dab04
617f3b6
f976d88
d795a86
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 | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -285,19 +285,6 @@ public RemoteClusterConnection getRemoteClusterConnection(String cluster) { | |||||||||
return connection; | ||||||||||
} | ||||||||||
|
||||||||||
@Override | ||||||||||
public void skipUnavailableChanged( | ||||||||||
ProjectId originProjectId, | ||||||||||
ProjectId linkedProjectId, | ||||||||||
String linkedProjectAlias, | ||||||||||
boolean skipUnavailable | ||||||||||
) { | ||||||||||
final var remote = getConnectionsMapForProject(originProjectId).get(linkedProjectAlias); | ||||||||||
if (remote != null) { | ||||||||||
remote.setSkipUnavailable(skipUnavailable); | ||||||||||
} | ||||||||||
} | ||||||||||
|
||||||||||
@FixForMultiProject(description = "Refactor as needed to support project specific changes to linked remotes.") | ||||||||||
public synchronized void updateRemoteClusterCredentials(Supplier<Settings> settingsSupplier, ActionListener<Void> listener) { | ||||||||||
final var projectId = projectResolver.getProjectId(); | ||||||||||
|
@@ -450,6 +437,9 @@ synchronized void updateRemoteCluster( | |||||||||
connectionMap.put(clusterAlias, remote); | ||||||||||
remote.ensureConnected(listener.map(ignored -> RemoteClusterConnectionStatus.RECONNECTED)); | ||||||||||
} else { | ||||||||||
if (crossProjectEnabled == false) { | ||||||||||
remote.setSkipUnavailable(config.skipUnavailable()); | ||||||||||
|
remote.setSkipUnavailable(config.skipUnavailable()); | |
if (remote.isSkipUnavailable() != config.skipUnavailable()) { | |
remote.setSkipUnavailable(config.skipUnavailable()); | |
} |
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.
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.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -51,6 +51,7 @@ | |
import java.util.concurrent.CountDownLatch; | ||
import java.util.concurrent.TimeUnit; | ||
import java.util.concurrent.atomic.AtomicReference; | ||
import java.util.function.BiConsumer; | ||
import java.util.function.BiFunction; | ||
|
||
import static org.elasticsearch.test.MockLog.assertThatLogger; | ||
|
@@ -111,6 +112,10 @@ private MockTransportService startTransport( | |
return RemoteClusterConnectionTests.startTransport(id, knownNodes, version, transportVersion, threadPool, settings); | ||
} | ||
|
||
private MockTransportService startTransport(final String id) { | ||
return startTransport(id, List.of(), VersionInformation.CURRENT, TransportVersion.current(), Settings.EMPTY); | ||
} | ||
|
||
public void testSettingsAreRegistered() { | ||
assertTrue(ClusterSettings.BUILT_IN_CLUSTER_SETTINGS.contains(RemoteClusterSettings.REMOTE_CLUSTER_SKIP_UNAVAILABLE)); | ||
assertTrue(ClusterSettings.BUILT_IN_CLUSTER_SETTINGS.contains(RemoteClusterSettings.REMOTE_INITIAL_CONNECTION_TIMEOUT_SETTING)); | ||
|
@@ -1828,6 +1833,48 @@ public void testLogsConnectionResult() throws IOException { | |
} | ||
} | ||
|
||
public void testSetSkipUnavailable() throws IOException { | ||
final var skipUnavailableProperty = RemoteClusterSettings.REMOTE_CLUSTER_SKIP_UNAVAILABLE.getConcreteSettingForNamespace("remote") | ||
.getKey(); | ||
final var seedNodeProperty = SniffConnectionStrategySettings.REMOTE_CLUSTER_SEEDS.getConcreteSettingForNamespace("remote").getKey(); | ||
final var clusterSettings = ClusterSettings.createBuiltInClusterSettings(); | ||
|
||
try ( | ||
var remote1Transport = startTransport("remote1"); | ||
var remote2Transport = startTransport("remote2"); | ||
var local = startTransport("local"); | ||
var remoteClusterService = createRemoteClusterService(Settings.EMPTY, clusterSettings, local) | ||
) { | ||
linkedProjectConfigService.register(remoteClusterService); | ||
|
||
final BiConsumer<Boolean, MockTransportService> applySettingsAndVerify = (skipUnavailableValue, mockTransportService) -> { | ||
clusterSettings.applySettings( | ||
Settings.builder() | ||
.put(skipUnavailableProperty, skipUnavailableValue) | ||
.put(seedNodeProperty, mockTransportService.getLocalNode().getAddress().toString()) | ||
.build() | ||
); | ||
assertTrue(isRemoteClusterRegistered(remoteClusterService, "remote")); | ||
assertEquals(skipUnavailableValue, remoteClusterService.isSkipUnavailable("remote").orElseThrow()); | ||
}; | ||
|
||
// Apply the initial settings and verify the new connection is built. | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be great to somehow assert that the connection is the same instance, i.e. no rebuilding. |
||
skipUnavailable = skipUnavailable == false; | ||
applySettingsAndVerify.accept(skipUnavailable, remote1Transport); | ||
|
||
// Change the seed node but not skip_unavailable, connection should be rebuilt and skip_unavailable should stay the same. | ||
applySettingsAndVerify.accept(skipUnavailable, remote2Transport); | ||
|
||
// Change skip_unavailable value and the seed node, connection should be rebuilt and skip_unavailable should also be modified. | ||
skipUnavailable = skipUnavailable == false; | ||
applySettingsAndVerify.accept(skipUnavailable, remote1Transport); | ||
} | ||
} | ||
|
||
private void updateRemoteCluster( | ||
RemoteClusterService service, | ||
String alias, | ||
|
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.