add tests

Author: ywangd

modules/repository-gcs/src/main/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageClientsManager.java

@@ -41,8 +41,9 @@ public class GoogleCloudStorageClientsManager implements ClusterStateApplier {
         GcsRepositoryStatsCollector,
         MeteredStorage,
         IOException> clientBuilder;
-    private final ClusterClientsHolder clusterClientsHolder = new ClusterClientsHolder();
-    private final Map<ProjectId, ClientsHolder> projectClientHolders;
+    // A map of projectId to clients holder. Adding to and removing from the map happen only in the applier thread.
+    private final Map<ProjectId, ClientsHolder> perProjectClientsHolders;
+    private final ClusterClientsHolder clusterClientsHolder;
 
     public GoogleCloudStorageClientsManager(
         Settings nodeSettings,
@@ -55,15 +56,16 @@ public GoogleCloudStorageClientsManager(
             .build();
         this.clientBuilder = clientBuilder;
         if (supportsMultipleProjects) {
-            this.projectClientHolders = ConcurrentCollections.newConcurrentMap();
+            this.perProjectClientsHolders = ConcurrentCollections.newConcurrentMap();
         } else {
-            this.projectClientHolders = null;
+            this.perProjectClientsHolders = null;
         }
+        this.clusterClientsHolder = new ClusterClientsHolder();
     }
 
     @Override
     public void applyClusterState(ClusterChangedEvent event) {
-        assert projectClientHolders != null;
+        assert perProjectClientsHolders != null;
         final Map<ProjectId, ProjectMetadata> currentProjects = event.state().metadata().projects();
 
         final var updatedPerProjectClients = new HashMap<ProjectId, ClientsHolder>();
@@ -77,7 +79,7 @@ public void applyClusterState(ClusterChangedEvent event) {
             // Project secrets can be null when node restarts. It may not have any s3 credentials if s3 is not in use.
             if (projectSecrets == null || projectSecrets.getSettingNames().stream().noneMatch(key -> key.startsWith(GCS_SETTING_PREFIX))) {
                 // Most likely there won't be any existing client, but attempt to remove it anyway just in case
-                projectClientHolders.remove(project.id());
+                perProjectClientsHolders.remove(project.id());
                 continue;
             }
 
@@ -113,13 +115,13 @@ public void applyClusterState(ClusterChangedEvent event) {
         // Updated projects
         for (var projectId : updatedPerProjectClients.keySet()) {
             assert ProjectId.DEFAULT.equals(projectId) == false;
-            projectClientHolders.put(projectId, updatedPerProjectClients.get(projectId));
+            perProjectClientsHolders.put(projectId, updatedPerProjectClients.get(projectId));
         }
         // Removed projects
-        for (var projectId : projectClientHolders.keySet()) {
+        for (var projectId : perProjectClientsHolders.keySet()) {
             if (currentProjects.containsKey(projectId) == false) {
                 assert ProjectId.DEFAULT.equals(projectId) == false;
-                projectClientHolders.remove(projectId);
+                perProjectClientsHolders.remove(projectId);
             }
         }
     }
@@ -140,23 +142,41 @@ void refreshAndClearCacheForClusterClients(Map<String, GoogleCloudStorageClientS
 
     MeteredStorage client(ProjectId projectId, String clientName, String repositoryName, GcsRepositoryStatsCollector statsCollector)
         throws IOException {
-        return getClientsHolderSafe(projectId).client(clientName, repositoryName, statsCollector);
+        if (projectId == null || ProjectId.DEFAULT.equals(projectId)) {
+            return clusterClientsHolder.client(clientName, repositoryName, statsCollector);
+        } else {
+            return getClientsHolderSafe(projectId).client(clientName, repositoryName, statsCollector);
+        }
     }
 
     void closeRepositoryClients(ProjectId projectId, String repositoryName) {
-        getClientsHolderSafe(projectId).closeRepositoryClients(repositoryName);
+        if (projectId == null || ProjectId.DEFAULT.equals(projectId)) {
+            clusterClientsHolder.closeRepositoryClients(repositoryName);
+        } else {
+            getClientsHolderSafe(projectId).closeRepositoryClients(repositoryName);
+        }
+    }
+
+    // package private for tests
+    ClusterClientsHolder getClusterClientsHolder() {
+        return clusterClientsHolder;
+    }
+
+    // package private for tests
+    Map<ProjectId, ClientsHolder> getPerProjectClientsHolders() {
+        return perProjectClientsHolders == null ? null : Map.copyOf(perProjectClientsHolders);
     }
 
     private boolean newOrUpdated(ProjectId projectId, Map<String, GoogleCloudStorageClientSettings> currentClientSettings) {
-        final var old = projectClientHolders.get(projectId);
+        final var old = perProjectClientsHolders.get(projectId);
         if (old == null) {
             return true;
         }
-        return currentClientSettings.equals(old.getClientSettings()) == false;
+        return currentClientSettings.equals(old.allClientSettings()) == false;
     }
 
     private ClientsHolder getClientsHolderSafe(ProjectId projectId) {
-        final var clientsHolder = projectClientHolders.get(projectId);
+        final var clientsHolder = perProjectClientsHolders.get(projectId);
         if (clientsHolder == null) {
             assert ProjectId.DEFAULT.equals(projectId) == false;
             throw new IllegalArgumentException("No GCS client is configured for project [" + projectId + "]");
@@ -175,7 +195,7 @@ abstract class ClientsHolder {
         /**
          * Get the current client settings for all clients in this holder.
          */
-        protected abstract Map<String, GoogleCloudStorageClientSettings> getClientSettings();
+        protected abstract Map<String, GoogleCloudStorageClientSettings> allClientSettings();
 
         /**
          * Attempts to retrieve a client from the cache. If the client does not exist it
@@ -204,14 +224,14 @@ MeteredStorage client(final String clientName, final String repositoryName, fina
                     return existing;
                 }
 
-                final GoogleCloudStorageClientSettings settings = getClientSettings().get(clientName);
+                final GoogleCloudStorageClientSettings settings = allClientSettings().get(clientName);
 
                 if (settings == null) {
                     throw new IllegalArgumentException(
                         "Unknown client name ["
                             + clientName
                             + "]. Existing client configs: "
-                            + Strings.collectionToDelimitedString(getClientSettings().keySet(), ",")
+                            + Strings.collectionToDelimitedString(allClientSettings().keySet(), ",")
                     );
                 }
 
@@ -228,14 +248,19 @@ synchronized void closeRepositoryClients(String repositoryName) {
                 .filter(entry -> entry.getKey().equals(repositoryName) == false)
                 .collect(Collectors.toUnmodifiableMap(Map.Entry::getKey, java.util.Map.Entry::getValue));
         }
+
+        // package private for tests
+        final boolean hasCachedClientForRepository(String repositoryName) {
+            return clientCache.containsKey(repositoryName);
+        }
     }
 
     final class ClusterClientsHolder extends ClientsHolder {
 
         private volatile Map<String, GoogleCloudStorageClientSettings> clientSettings = emptyMap();
 
         @Override
-        protected Map<String, GoogleCloudStorageClientSettings> getClientSettings() {
+        protected Map<String, GoogleCloudStorageClientSettings> allClientSettings() {
             return clientSettings;
         }
 
@@ -261,7 +286,7 @@ final class PerProjectClientsHolder extends ClientsHolder {
         }
 
         @Override
-        protected Map<String, GoogleCloudStorageClientSettings> getClientSettings() {
+        protected Map<String, GoogleCloudStorageClientSettings> allClientSettings() {
             return clientSettings;
         }
     }

modules/repository-gcs/src/main/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageService.java

@@ -25,6 +25,7 @@
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
 import org.elasticsearch.ExceptionsHelper;
+import org.elasticsearch.cluster.metadata.ProjectId;
 import org.elasticsearch.cluster.node.DiscoveryNode;
 import org.elasticsearch.cluster.project.ProjectResolver;
 import org.elasticsearch.cluster.service.ClusterService;
@@ -97,15 +98,47 @@ public void refreshAndClearCache(Map<String, GoogleCloudStorageClientSettings> c
      * @return a cached client storage instance that can be used to manage objects
      *         (blobs)
      */
+    @Deprecated(forRemoval = true)
     public MeteredStorage client(final String clientName, final String repositoryName, final GcsRepositoryStatsCollector statsCollector)
         throws IOException {
         return clientsManager.client(clientName, repositoryName, statsCollector);
     }
 
+    /**
+     * Attempts to retrieve a client from the cache. If the client does not exist it
+     * will be created from the latest settings and will populate the cache. The
+     * returned instance should not be cached by the calling code. Instead, for each
+     * use, the (possibly updated) instance should be requested by calling this
+     * method.
+     *
+     * @param clientName name of the client settings used to create the client
+     * @param repositoryName name of the repository that would use the client
+     * @return a cached client storage instance that can be used to manage objects
+     *         (blobs)
+     */
+    public MeteredStorage client(
+        final ProjectId projectId,
+        final String clientName,
+        final String repositoryName,
+        final GcsRepositoryStatsCollector statsCollector
+    ) throws IOException {
+        return clientsManager.client(projectId, clientName, repositoryName, statsCollector);
+    }
+
+    @Deprecated(forRemoval = true)
     void closeRepositoryClients(String repositoryName) {
         clientsManager.closeRepositoryClients(repositoryName);
     }
 
+    void closeRepositoryClients(final ProjectId projectId, String repositoryName) {
+        clientsManager.closeRepositoryClients(projectId, repositoryName);
+    }
+
+    // package-private for tests
+    GoogleCloudStorageClientsManager getClientsManager() {
+        return clientsManager;
+    }
+
     /**
      * Creates a client that can be used to manage Google Cloud Storage objects. The client is thread-safe.
      *