No need to sort

n1v0lg · Jan 8, 2024 · 114816e · 114816e
1 parent 0aa58da
commit 114816e
Show file tree

Hide file tree

Showing 3 changed files with 18 additions and 24 deletions.
diff --git a/server/src/main/java/org/elasticsearch/transport/RemoteClusterCredentialsManager.java b/server/src/main/java/org/elasticsearch/transport/RemoteClusterCredentialsManager.java
@@ -18,7 +18,7 @@
 
 import java.util.Collections;
 import java.util.Map;
-import java.util.SortedSet;
+import java.util.Set;
 import java.util.TreeSet;
 
 import static org.elasticsearch.transport.RemoteClusterService.REMOTE_CLUSTER_CREDENTIALS;
@@ -40,19 +40,14 @@ public final synchronized UpdateRemoteClusterCredentialsResult updateClusterCred
             return new UpdateRemoteClusterCredentialsResult(new TreeSet<>(newClusterCredentials.keySet()), Collections.emptySortedSet());
         }
 
-        final SortedSet<String> addedClusterAliases = Sets.sortedDifference(newClusterCredentials.keySet(), clusterCredentials.keySet());
-        final SortedSet<String> removedClusterAliases = Sets.sortedDifference(clusterCredentials.keySet(), newClusterCredentials.keySet());
+        final Set<String> addedClusterAliases = Sets.difference(newClusterCredentials.keySet(), clusterCredentials.keySet());
+        final Set<String> removedClusterAliases = Sets.difference(clusterCredentials.keySet(), newClusterCredentials.keySet());
         setClusterCredentialsAndLog(newClusterCredentials);
         assert Sets.haveEmptyIntersection(removedClusterAliases, addedClusterAliases);
         return new UpdateRemoteClusterCredentialsResult(addedClusterAliases, removedClusterAliases);
     }
 
-    public record UpdateRemoteClusterCredentialsResult(
-        // Use sorted sets since we will iterate over these, and call a synchronized method for each.
-        // Sorting establishes a deterministic call order to prevent deadlocks
-        SortedSet<String> addedClusterAliases,
-        SortedSet<String> removedClusterAliases
-    ) {}
+    public record UpdateRemoteClusterCredentialsResult(Set<String> addedClusterAliases, Set<String> removedClusterAliases) {}
 
     @Nullable
     public SecureString resolveCredentials(String clusterAlias) {

diff --git a/server/src/main/java/org/elasticsearch/transport/RemoteClusterService.java b/server/src/main/java/org/elasticsearch/transport/RemoteClusterService.java
@@ -308,11 +308,7 @@ private synchronized void updateSkipUnavailable(String clusterAlias, Boolean ski
     }
 
     public synchronized void updateRemoteClusterCredentials(Supplier<Settings> settingsSupplier, ActionListener<Void> listener) {
-        updateRemoteClusterCredentials(settingsSupplier.get(), listener);
-    }
-
-    // package-private for testing
-    synchronized void updateRemoteClusterCredentials(Settings settings, ActionListener<Void> listener) {
+        final Settings settings = settingsSupplier.get();
         final UpdateRemoteClusterCredentialsResult result = remoteClusterCredentialsManager.updateClusterCredentials(settings);
         // We only need to rebuild connections when a credential was newly added or removed for a cluster alias, not if the credential
         // value was updated. Therefore, only consider added or removed aliases
@@ -333,6 +329,8 @@ synchronized void updateRemoteClusterCredentials(Settings settings, ActionListen
         }
     }
 
+    // package-private for testing
+
     private void maybeRebuildConnectionOnCredentialsChange(String clusterAlias, Settings settings, RefCountingRunnable connectionRefs) {
         if (false == remoteClusters.containsKey(clusterAlias)) {
             // A credential was added or removed before a remote connection was configured.

diff --git a/server/src/test/java/org/elasticsearch/transport/RemoteClusterServiceTests.java b/server/src/test/java/org/elasticsearch/transport/RemoteClusterServiceTests.java
@@ -1467,10 +1467,8 @@ public void testUpdateRemoteClusterCredentialsRebuildsConnectionWithCorrectProfi
                         final MockSecureSettings secureSettings = new MockSecureSettings();
                         secureSettings.setString("cluster.remote.cluster_1.credentials", randomAlphaOfLength(10));
                         final PlainActionFuture<Void> listener = new PlainActionFuture<>();
-                        service.updateRemoteClusterCredentials(
-                            Settings.builder().put(clusterSettings).setSecureSettings(secureSettings).build(),
-                            listener
-                        );
+                        final Settings settings = Settings.builder().put(clusterSettings).setSecureSettings(secureSettings).build();
+                        service.updateRemoteClusterCredentials(() -> settings, listener);
                         listener.actionGet(10, TimeUnit.SECONDS);
                     }
 
@@ -1483,7 +1481,7 @@ public void testUpdateRemoteClusterCredentialsRebuildsConnectionWithCorrectProfi
                         final PlainActionFuture<Void> listener = new PlainActionFuture<>();
                         service.updateRemoteClusterCredentials(
                             // Settings without credentials constitute credentials removal
-                            clusterSettings,
+                            () -> clusterSettings,
                             listener
                         );
                         listener.actionGet(10, TimeUnit.SECONDS);
@@ -1568,10 +1566,12 @@ public void testUpdateRemoteClusterCredentialsRebuildsMultipleConnectionsDespite
                         secureSettings.setString("cluster.remote." + goodCluster + ".credentials", randomAlphaOfLength(10));
                         secureSettings.setString("cluster.remote." + missingCluster + ".credentials", randomAlphaOfLength(10));
                         final PlainActionFuture<Void> listener = new PlainActionFuture<>();
-                        service.updateRemoteClusterCredentials(
-                            Settings.builder().put(cluster1Settings).put(cluster2Settings).setSecureSettings(secureSettings).build(),
-                            listener
-                        );
+                        final Settings settings = Settings.builder()
+                            .put(cluster1Settings)
+                            .put(cluster2Settings)
+                            .setSecureSettings(secureSettings)
+                            .build();
+                        service.updateRemoteClusterCredentials(() -> settings, listener);
                         listener.actionGet(10, TimeUnit.SECONDS);
                     }
 
@@ -1587,9 +1587,10 @@ public void testUpdateRemoteClusterCredentialsRebuildsMultipleConnectionsDespite
 
                     {
                         final PlainActionFuture<Void> listener = new PlainActionFuture<>();
+                        final Settings settings = Settings.builder().put(cluster1Settings).put(cluster2Settings).build();
                         service.updateRemoteClusterCredentials(
                             // Settings without credentials constitute credentials removal
-                            Settings.builder().put(cluster1Settings).put(cluster2Settings).build(),
+                            () -> settings,
                             listener
                         );
                         listener.actionGet(10, TimeUnit.SECONDS);