address comments

linkedin · Apr 25, 2024 · 30d2d49 · 30d2d49
1 parent 07261b5
commit 30d2d49
Show file tree

Hide file tree

Showing 2 changed files with 22 additions and 21 deletions.
diff --git a/cluster/storage/src/main/java/com/linkedin/openhouse/cluster/storage/BaseStorage.java b/cluster/storage/src/main/java/com/linkedin/openhouse/cluster/storage/BaseStorage.java
@@ -19,7 +19,7 @@ public abstract class BaseStorage implements Storage {
    *
    * <p>The storage is considered configured if type is defined in the storage properties.
    *
-   * @return true if the local storage is configured, false otherwise
+   * @return true if the storage is configured, false otherwise
    */
   @Override
   public boolean isConfigured() {

diff --git a/cluster/storage/src/main/java/com/linkedin/openhouse/cluster/storage/StorageManager.java b/cluster/storage/src/main/java/com/linkedin/openhouse/cluster/storage/StorageManager.java
@@ -10,6 +10,8 @@
 import javax.annotation.PostConstruct;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.stereotype.Component;
+import org.springframework.util.CollectionUtils;
+import org.springframework.util.StringUtils;
 
 /**
  * The StorageManager class is responsible for managing the storage types and providing the
@@ -22,37 +24,36 @@ public class StorageManager {
 
   @Autowired StorageType storageType;
 
-  @Autowired List<Storage> storagesBeans;
+  @Autowired List<Storage> storages;
 
   /**
    * Validate the storage properties.
    *
    * <p>It validates storage properties as follows
    *
-   * <p>1. default-type for storage is not set and storage types are not provided, ie. LocalStorage
-   * is configured **valid**
+   * <p>1. If any storage type is configured, then default type must be set. Alternatively, if a
+   * default type is not set, then storage types should also be null or empty. **valid**
    *
-   * <p>2. default-type for storage is set and storage types are provided ,and it contains
-   * default-type **valid**
+   * <p>2. If default-type is set, then the value of the default type must exist in the configured
+   * storage types. **valid**
    *
    * <p>all other configurations are **invalid**
    */
   @PostConstruct
   public void validateProperties() {
     String clusterYamlError = "Cluster yaml is incorrectly configured: ";
-    Preconditions.checkArgument(
-        !(storageProperties.getDefaultType() == null
-            && storageProperties.getTypes() != null
-            && !storageProperties.getTypes().isEmpty()),
-        clusterYamlError + "default-type for storage is not set");
-    Preconditions.checkArgument(
-        !(storageProperties.getDefaultType() != null
-            && (storageProperties.getTypes() == null || storageProperties.getTypes().isEmpty())),
-        "no types are provided");
-    Preconditions.checkArgument(
-        !(storageProperties.getDefaultType() != null
-            && !storageProperties.getTypes().containsKey(storageProperties.getDefaultType())),
-        "default-type was not provided in types");
+    if (!StringUtils.hasText(storageProperties.getDefaultType())) {
+      // default-type is not configured, types should be null or empty
+      Preconditions.checkArgument(
+          CollectionUtils.isEmpty(storageProperties.getTypes()),
+          clusterYamlError + "default-type is not set, storage types should not be provided");
+    } else {
+      // default-type is configured, types should contain the default-type
+      Preconditions.checkArgument(
+          !CollectionUtils.isEmpty(storageProperties.getTypes())
+              && storageProperties.getTypes().containsKey(storageProperties.getDefaultType()),
+          clusterYamlError + "storage types should contain the default-type");
+    }
     try {
       Optional.ofNullable(storageProperties.getDefaultType()).ifPresent(storageType::fromString);
       Optional.ofNullable(storageProperties.getTypes())
@@ -69,7 +70,7 @@ public void validateProperties() {
    * @return the default storage
    */
   public Storage getDefaultStorage() {
-    if (storageProperties.getDefaultType() == null) {
+    if (!StringUtils.hasText(storageProperties.getDefaultType())) {
       return getStorage(LOCAL);
     }
     return getStorage(storageType.fromString(storageProperties.getDefaultType()));
@@ -82,7 +83,7 @@ public Storage getDefaultStorage() {
    * @return the storage
    */
   public Storage getStorage(StorageType.Type storageType) {
-    for (Storage storage : storagesBeans) {
+    for (Storage storage : storages) {
       if (storage.getType().equals(storageType) && storage.isConfigured()) {
         return storage;
       }