Incorporate PR review feedback - 1

Signed-off-by: Bhumika Saini <[email protected]>
opensearch-project · Sep 28, 2023 · c605c62 · c605c62
1 parent 8216a25
commit c605c62
Show file tree

Hide file tree

Showing 7 changed files with 91 additions and 81 deletions.
diff --git a/plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3Repository.java b/plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3Repository.java
@@ -204,19 +204,19 @@ class S3Repository extends MeteredBlobStoreRepository {
 
     private final S3Service service;
 
-    private final String bucket;
+    private String bucket;
 
-    private final ByteSizeValue bufferSize;
+    private ByteSizeValue bufferSize;
 
-    private final ByteSizeValue chunkSize;
+    private ByteSizeValue chunkSize;
 
-    private final BlobPath basePath;
+    private BlobPath basePath;
 
-    private final boolean serverSideEncryption;
+    private boolean serverSideEncryption;
 
-    private final String storageClass;
+    private String storageClass;
 
-    private final String cannedACL;
+    private String cannedACL;
 
     private RepositoryMetadata repositoryMetadata;
 
@@ -282,66 +282,11 @@ class S3Repository extends MeteredBlobStoreRepository {
         this.s3AsyncService = s3AsyncService;
         this.multipartUploadEnabled = multipartUploadEnabled;
         this.pluginConfigPath = pluginConfigPath;
-
-        this.repositoryMetadata = metadata;
         this.asyncUploadUtils = asyncUploadUtils;
         this.priorityExecutorBuilder = priorityExecutorBuilder;
         this.normalExecutorBuilder = normalExecutorBuilder;
 
-        // Parse and validate the user's S3 Storage Class setting
-        this.bucket = BUCKET_SETTING.get(metadata.settings());
-        if (bucket == null) {
-            throw new RepositoryException(metadata.name(), "No bucket defined for s3 repository");
-        }
-
-        this.bufferSize = BUFFER_SIZE_SETTING.get(metadata.settings());
-        this.chunkSize = CHUNK_SIZE_SETTING.get(metadata.settings());
-
-        // We make sure that chunkSize is bigger or equal than/to bufferSize
-        if (this.chunkSize.getBytes() < bufferSize.getBytes()) {
-            throw new RepositoryException(
-                metadata.name(),
-                CHUNK_SIZE_SETTING.getKey()
-                    + " ("
-                    + this.chunkSize
-                    + ") can't be lower than "
-                    + BUFFER_SIZE_SETTING.getKey()
-                    + " ("
-                    + bufferSize
-                    + ")."
-            );
-        }
-
-        final String basePath = BASE_PATH_SETTING.get(metadata.settings());
-        if (Strings.hasLength(basePath)) {
-            this.basePath = new BlobPath().add(basePath);
-        } else {
-            this.basePath = BlobPath.cleanPath();
-        }
-
-        this.serverSideEncryption = SERVER_SIDE_ENCRYPTION_SETTING.get(metadata.settings());
-
-        this.storageClass = STORAGE_CLASS_SETTING.get(metadata.settings());
-        this.cannedACL = CANNED_ACL_SETTING.get(metadata.settings());
-
-        if (S3ClientSettings.checkDeprecatedCredentials(metadata.settings())) {
-            // provided repository settings
-            deprecationLogger.deprecate(
-                "s3_repository_secret_settings",
-                "Using s3 access/secret key from repository settings. Instead "
-                    + "store these in named clients and the opensearch keystore for secure settings."
-            );
-        }
-
-        logger.debug(
-            "using bucket [{}], chunk_size [{}], server_side_encryption [{}], buffer_size [{}], cannedACL [{}], storageClass [{}]",
-            bucket,
-            chunkSize,
-            serverSideEncryption,
-            bufferSize,
-            cannedACL,
-            storageClass
-        );
+        readRepositoryMetadata(metadata);
     }
 
     private static Map<String, String> buildLocation(RepositoryMetadata metadata) {
@@ -420,10 +365,11 @@ public boolean isReloadable() {
     }
 
     @Override
-    public void reload(RepositoryMetadata newRepositoryMetadata, boolean compress) {
+    public void reload(RepositoryMetadata newRepositoryMetadata) {
         // Reload configs for S3Repository
-        super.reload(newRepositoryMetadata, compress);
+        super.reload(newRepositoryMetadata);
         repositoryMetadata = newRepositoryMetadata;
+        readRepositoryMetadata(repositoryMetadata);
 
         // Reload configs for S3RepositoryPlugin
         final Map<String, S3ClientSettings> clientsSettings = S3ClientSettings.load(repositoryMetadata.settings(), pluginConfigPath);
@@ -432,7 +378,71 @@ public void reload(RepositoryMetadata newRepositoryMetadata, boolean compress) {
 
         // Reload configs for S3BlobStore
         BlobStore blobStore = getBlobStore();
-        blobStore.reload(newRepositoryMetadata);
+        blobStore.reload(repositoryMetadata);
+    }
+
+    /**
+     * Reloads the values derived from the Repository Metadata
+     *
+     * @param repositoryMetadata RepositoryMetadata instance to derive the values from
+     */
+    private void readRepositoryMetadata(RepositoryMetadata repositoryMetadata) {
+        this.repositoryMetadata = metadata;
+
+        // Parse and validate the user's S3 Storage Class setting
+        this.bucket = BUCKET_SETTING.get(metadata.settings());
+        if (bucket == null) {
+            throw new RepositoryException(metadata.name(), "No bucket defined for s3 repository");
+        }
+
+        this.bufferSize = BUFFER_SIZE_SETTING.get(metadata.settings());
+        this.chunkSize = CHUNK_SIZE_SETTING.get(metadata.settings());
+
+        // We make sure that chunkSize is bigger or equal than/to bufferSize
+        if (this.chunkSize.getBytes() < bufferSize.getBytes()) {
+            throw new RepositoryException(
+                metadata.name(),
+                CHUNK_SIZE_SETTING.getKey()
+                    + " ("
+                    + this.chunkSize
+                    + ") can't be lower than "
+                    + BUFFER_SIZE_SETTING.getKey()
+                    + " ("
+                    + bufferSize
+                    + ")."
+            );
+        }
+
+        final String basePath = BASE_PATH_SETTING.get(metadata.settings());
+        if (Strings.hasLength(basePath)) {
+            this.basePath = new BlobPath().add(basePath);
+        } else {
+            this.basePath = BlobPath.cleanPath();
+        }
+
+        this.serverSideEncryption = SERVER_SIDE_ENCRYPTION_SETTING.get(metadata.settings());
+
+        this.storageClass = STORAGE_CLASS_SETTING.get(metadata.settings());
+        this.cannedACL = CANNED_ACL_SETTING.get(metadata.settings());
+
+        if (S3ClientSettings.checkDeprecatedCredentials(metadata.settings())) {
+            // provided repository settings
+            deprecationLogger.deprecate(
+                "s3_repository_secret_settings",
+                "Using s3 access/secret key from repository settings. Instead "
+                    + "store these in named clients and the opensearch keystore for secure settings."
+            );
+        }
+
+        logger.debug(
+            "using bucket [{}], chunk_size [{}], server_side_encryption [{}], buffer_size [{}], cannedACL [{}], storageClass [{}]",
+            bucket,
+            chunkSize,
+            serverSideEncryption,
+            bufferSize,
+            cannedACL,
+            storageClass
+        );
     }
 
     @Override

diff --git a/server/src/main/java/org/opensearch/repositories/RepositoriesService.java b/server/src/main/java/org/opensearch/repositories/RepositoriesService.java
@@ -83,7 +83,6 @@
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
 
-import static org.opensearch.repositories.blobstore.BlobStoreRepository.COMPRESS_SETTING;
 import static org.opensearch.repositories.blobstore.BlobStoreRepository.REMOTE_STORE_INDEX_SHALLOW_COPY;
 
 /**
@@ -459,7 +458,7 @@ public void applyClusterState(ClusterChangedEvent event) {
                             // Previous version is different from the version in settings
                             logger.debug("updating repository [{}]", repositoryMetadata.name());
                             if (repository.isSystemRepository() && repository.isReloadable()) {
-                                repository.reload(repositoryMetadata, COMPRESS_SETTING.get(repositoryMetadata.settings()));
+                                repository.reload(repositoryMetadata);
                             } else {
                                 closeRepository(repository);
                                 archiveRepositoryStats(repository, state.version());

diff --git a/server/src/main/java/org/opensearch/repositories/Repository.java b/server/src/main/java/org/opensearch/repositories/Repository.java
@@ -452,5 +452,5 @@ default boolean isReloadable() {
     /**
      * Reload the repository inplace
      */
-    default void reload(RepositoryMetadata repositoryMetadata, boolean compress) {}
+    default void reload(RepositoryMetadata repositoryMetadata) {}
 }
diff --git a/server/src/main/java/org/opensearch/repositories/blobstore/BlobStoreRepository.java b/server/src/main/java/org/opensearch/repositories/blobstore/BlobStoreRepository.java
@@ -413,7 +413,7 @@ protected BlobStoreRepository(
         final RecoverySettings recoverySettings
     ) {
         // Read RepositoryMetadata as the first step
-        readRepositoryMetadata(repositoryMetadata, compress);
+        readRepositoryMetadata(repositoryMetadata);
 
         isSystemRepository = SYSTEM_REPOSITORY_SETTING.get(metadata.settings());
         this.namedXContentRegistry = namedXContentRegistry;
@@ -423,17 +423,16 @@ protected BlobStoreRepository(
     }
 
     @Override
-    public void reload(RepositoryMetadata repositoryMetadata, boolean compress) {
-        readRepositoryMetadata(repositoryMetadata, compress);
+    public void reload(RepositoryMetadata repositoryMetadata) {
+        readRepositoryMetadata(repositoryMetadata);
     }
 
     /**
      * Reloads the values derived from the Repository Metadata
      *
      * @param repositoryMetadata RepositoryMetadata instance to derive the values from
-     * @param compress boolean representing whether compression is to be used
      */
-    private void readRepositoryMetadata(RepositoryMetadata repositoryMetadata, boolean compress) {
+    private void readRepositoryMetadata(RepositoryMetadata repositoryMetadata) {
         this.metadata = repositoryMetadata;
 
         supportURLRepo = SUPPORT_URL_REPO.get(metadata.settings());
@@ -445,7 +444,9 @@ private void readRepositoryMetadata(RepositoryMetadata repositoryMetadata, boole
         cacheRepositoryData = CACHE_REPOSITORY_DATA.get(metadata.settings());
         bufferSize = Math.toIntExact(BUFFER_SIZE_SETTING.get(metadata.settings()).getBytes());
         maxShardBlobDeleteBatch = MAX_SNAPSHOT_SHARD_BLOB_DELETE_BATCH_SIZE.get(metadata.settings());
-        compressor = compress ? COMPRESSION_TYPE_SETTING.get(metadata.settings()) : CompressorRegistry.none();
+        compressor = COMPRESS_SETTING.get(metadata.settings())
+            ? COMPRESSION_TYPE_SETTING.get(metadata.settings())
+            : CompressorRegistry.none();
     }
 
     @Override

diff --git a/server/src/main/java/org/opensearch/repositories/blobstore/MeteredBlobStoreRepository.java b/server/src/main/java/org/opensearch/repositories/blobstore/MeteredBlobStoreRepository.java
@@ -71,8 +71,8 @@ public MeteredBlobStoreRepository(
     }
 
     @Override
-    public void reload(RepositoryMetadata repositoryMetadata, boolean compress) {
-        super.reload(repositoryMetadata, compress);
+    public void reload(RepositoryMetadata repositoryMetadata) {
+        super.reload(repositoryMetadata);
 
         // Not adding any additional reload logic here is intentional as the constructor only
         // initializes the repositoryInfo from the repo metadata, which cannot be changed.

diff --git a/server/src/main/java/org/opensearch/repositories/fs/ReloadableFsRepository.java b/server/src/main/java/org/opensearch/repositories/fs/ReloadableFsRepository.java
@@ -54,8 +54,8 @@ public boolean isReloadable() {
     }
 
     @Override
-    public void reload(RepositoryMetadata repositoryMetadata, boolean compress) {
-        super.reload(repositoryMetadata, compress);
+    public void reload(RepositoryMetadata repositoryMetadata) {
+        super.reload(repositoryMetadata);
         metadata = repositoryMetadata;
 
         // TODO - deduplicate the below block

diff --git a/server/src/test/java/org/opensearch/repositories/fs/ReloadableFsRepositoryTests.java b/server/src/test/java/org/opensearch/repositories/fs/ReloadableFsRepositoryTests.java
@@ -69,7 +69,7 @@ public void testIsReloadable() {
     public void testCompressReload() {
         assertEquals(CompressorRegistry.none(), repository.getCompressor());
         updateCompressionTypeToDefault();
-        repository.reload(metadata, true);
+        repository.reload(metadata);
         assertEquals(CompressorRegistry.defaultCompressor(), repository.getCompressor());
     }
 
@@ -99,7 +99,7 @@ public void testCompressionTypeReload() {
             .put(FsRepository.BASE_PATH_SETTING.getKey(), "my_base_path")
             .build();
         metadata = new RepositoryMetadata("test", "fs", settings);
-        repository.reload(metadata, true);
+        repository.reload(metadata);
         assertEquals(CompressorRegistry.getCompressor(ZstdCompressor.NAME.toUpperCase(Locale.ROOT)), repository.getCompressor());
     }