fix the review

hypertrace · Oct 12, 2023 · a445d4e · a445d4e
1 parent d6daf46
commit a445d4e
Show file tree

Hide file tree

Showing 6 changed files with 29 additions and 51 deletions.
diff --git a/config-service-factory/build.gradle.kts b/config-service-factory/build.gradle.kts
@@ -6,7 +6,7 @@ dependencies {
   api(libs.hypertrace.grpc.framework)
 
   implementation(projects.configServiceChangeEventGenerator)
-
+  implementation(libs.hypertrace.documentstore)
   implementation(projects.configServiceImpl)
   implementation(projects.spacesConfigServiceImpl)
   implementation(projects.labelsConfigServiceImpl)

diff --git a/config-service-factory/src/main/java/org/hypertrace/config/service/ConfigServiceFactory.java b/config-service-factory/src/main/java/org/hypertrace/config/service/ConfigServiceFactory.java
@@ -14,6 +14,8 @@
 import org.hypertrace.config.service.change.event.impl.ConfigChangeEventGeneratorFactory;
 import org.hypertrace.config.service.store.ConfigStore;
 import org.hypertrace.config.service.store.DocumentConfigStore;
+import org.hypertrace.core.documentstore.Datastore;
+import org.hypertrace.core.documentstore.DatastoreProvider;
 import org.hypertrace.core.serviceframework.grpc.GrpcPlatformService;
 import org.hypertrace.core.serviceframework.grpc.GrpcPlatformServiceFactory;
 import org.hypertrace.core.serviceframework.grpc.GrpcServiceContainerEnvironment;
@@ -28,6 +30,8 @@ public class ConfigServiceFactory implements GrpcPlatformServiceFactory {
   private static final String SERVICE_NAME = "config-service";
   private static final String STORE_REPORTING_NAME = "config-service-store";
   private static final String GENERIC_CONFIG_SERVICE_CONFIG = "generic.config.service";
+  private static final String DOC_STORE_CONFIG_KEY = "document.store";
+  private static final String DATA_STORE_TYPE = "dataStoreType";
 
   private ConfigStore store;
   private GrpcServiceContainerEnvironment grpcServiceContainerEnvironment;
@@ -86,12 +90,19 @@ protected ConfigChangeEventGenerator buildChangeEventGenerator(Config config) {
 
   protected ConfigStore buildConfigStore(Config config) {
     try {
-      ConfigStore configStore = new DocumentConfigStore();
-      configStore.init(config.getConfig(GENERIC_CONFIG_SERVICE_CONFIG));
+      Datastore datastore = initDataStore(config.getConfig(GENERIC_CONFIG_SERVICE_CONFIG));
+      ConfigStore configStore = new DocumentConfigStore(datastore);
       this.store = configStore;
       return configStore;
     } catch (Exception e) {
       throw new RuntimeException("Error in getting or initializing config store", e);
     }
   }
+
+  private Datastore initDataStore(Config config) {
+    Config docStoreConfig = config.getConfig(DOC_STORE_CONFIG_KEY);
+    String dataStoreType = docStoreConfig.getString(DATA_STORE_TYPE);
+    Config dataStoreConfig = docStoreConfig.getConfig(dataStoreType);
+    return DatastoreProvider.getDatastore(dataStoreType, dataStoreConfig);
+  }
 }
diff --git a/config-service-impl/src/main/java/org/hypertrace/config/service/ConfigServiceGrpcImpl.java b/config-service-impl/src/main/java/org/hypertrace/config/service/ConfigServiceGrpcImpl.java
@@ -72,23 +72,23 @@ public void getConfig(
       GetConfigRequest request, StreamObserver<GetConfigResponse> responseObserver) {
     try {
       ConfigResourceContext configResourceContext = getConfigResourceContext(request);
-      Optional<ContextSpecificConfig> maybeConfig = configStore.getConfig(configResourceContext);
-      // initialize empty config if not present to handle merges better
-      if (maybeConfig.isEmpty()) {
-        maybeConfig = Optional.of(emptyConfig(configResourceContext.getContext()));
-      }
+      ContextSpecificConfig config =
+          configStore
+              .getConfig(configResourceContext)
+              .orElse(emptyConfig(configResourceContext.getContext()));
       // get the configs for the contexts mentioned in the request and merge them in the specified
       // order
       for (String context : request.getContextsList()) {
-        Optional<ContextSpecificConfig> contextSpecificConfig =
+        Optional<ContextSpecificConfig> maybeContextConfig =
             configStore.getConfig(getConfigResourceContext(request, context));
-        if (contextSpecificConfig.isEmpty()) {
+        if (maybeContextConfig.isEmpty()) {
           continue;
         }
-        maybeConfig = Optional.of(merge(maybeConfig.get(), contextSpecificConfig.get()));
+
+        config = merge(config, maybeContextConfig.get());
       }
 
-      filterNull(maybeConfig.get())
+      filterNull(config)
           .map(
               nonNullConfig ->
                   GetConfigResponse.newBuilder()

diff --git a/config-service-impl/src/main/java/org/hypertrace/config/service/store/ConfigStore.java b/config-service-impl/src/main/java/org/hypertrace/config/service/store/ConfigStore.java
@@ -1,7 +1,6 @@
 package org.hypertrace.config.service.store;
 
 import com.google.protobuf.Value;
-import com.typesafe.config.Config;
 import java.io.IOException;
 import java.util.Collection;
 import java.util.List;
@@ -17,14 +16,6 @@
  * resources.
  */
 public interface ConfigStore {
-
-  /**
-   * Initialize the config store
-   *
-   * @param config
-   */
-  void init(Config config);
-
   /**
    * Write the config value associated with the specified config resource to the store.
    *

diff --git a/...g-service-impl/src/main/java/org/hypertrace/config/service/store/DocumentConfigStore.java b/...g-service-impl/src/main/java/org/hypertrace/config/service/store/DocumentConfigStore.java
@@ -7,10 +7,8 @@
 import static org.hypertrace.config.service.store.ConfigDocument.TENANT_ID_FIELD_NAME;
 import static org.hypertrace.config.service.store.ConfigDocument.VERSION_FIELD_NAME;
 
-import com.google.common.annotations.VisibleForTesting;
 import com.google.common.collect.Maps;
 import com.google.protobuf.Value;
-import com.typesafe.config.Config;
 import io.grpc.Status;
 import java.io.IOException;
 import java.time.Clock;
@@ -33,7 +31,6 @@
 import org.hypertrace.core.documentstore.CloseableIterator;
 import org.hypertrace.core.documentstore.Collection;
 import org.hypertrace.core.documentstore.Datastore;
-import org.hypertrace.core.documentstore.DatastoreProvider;
 import org.hypertrace.core.documentstore.Document;
 import org.hypertrace.core.documentstore.Filter;
 import org.hypertrace.core.documentstore.Key;
@@ -43,42 +40,22 @@
 /** Document store which stores and serves the configurations. */
 @Slf4j
 public class DocumentConfigStore implements ConfigStore {
-
-  static final String DOC_STORE_CONFIG_KEY = "document.store";
-  static final String DATA_STORE_TYPE = "dataStoreType";
   static final String CONFIGURATIONS_COLLECTION = "configurations";
   private final Clock clock;
 
-  private Datastore datastore;
-  private Collection collection;
+  private final Datastore datastore;
+  private final Collection collection;
 
-  public DocumentConfigStore() {
-    this(Clock.systemUTC());
+  public DocumentConfigStore(Datastore datastore) {
+    this(Clock.systemUTC(), datastore);
   }
 
-  DocumentConfigStore(Clock clock) {
+  DocumentConfigStore(Clock clock, Datastore datastore) {
     this.clock = clock;
-  }
-
-  @Override
-  public void init(Config config) {
-    datastore = initDataStore(config);
-    this.collection = this.datastore.getCollection(CONFIGURATIONS_COLLECTION);
-  }
-
-  @VisibleForTesting
-  void initDatastore(Datastore datastore) {
     this.datastore = datastore;
     this.collection = this.datastore.getCollection(CONFIGURATIONS_COLLECTION);
   }
 
-  private Datastore initDataStore(Config config) {
-    Config docStoreConfig = config.getConfig(DOC_STORE_CONFIG_KEY);
-    String dataStoreType = docStoreConfig.getString(DATA_STORE_TYPE);
-    Config dataStoreConfig = docStoreConfig.getConfig(dataStoreType);
-    return DatastoreProvider.getDatastore(dataStoreType, dataStoreConfig);
-  }
-
   @Override
   public UpsertedConfig writeConfig(
       ConfigResourceContext configResourceContext, String userId, Value latestConfig)

diff --git a/...rvice-impl/src/test/java/org/hypertrace/config/service/store/DocumentConfigStoreTest.java b/...rvice-impl/src/test/java/org/hypertrace/config/service/store/DocumentConfigStoreTest.java
@@ -58,8 +58,7 @@ class DocumentConfigStoreTest {
   void beforeEach() {
     collection = mock(Collection.class);
     this.mockClock = mock(Clock.class);
-    this.configStore = new DocumentConfigStore(mockClock);
-    this.configStore.initDatastore(new MockDatastore());
+    this.configStore = new DocumentConfigStore(mockClock, new MockDatastore());
   }
 
   @Test