From a445d4e8414f4623718f44b26595ab671cd48662 Mon Sep 17 00:00:00 2001 From: aman-bansal Date: Thu, 12 Oct 2023 21:56:09 +0530 Subject: [PATCH] fix the review --- config-service-factory/build.gradle.kts | 2 +- .../config/service/ConfigServiceFactory.java | 15 +++++++-- .../config/service/ConfigServiceGrpcImpl.java | 18 +++++----- .../config/service/store/ConfigStore.java | 9 ----- .../service/store/DocumentConfigStore.java | 33 +++---------------- .../store/DocumentConfigStoreTest.java | 3 +- 6 files changed, 29 insertions(+), 51 deletions(-) diff --git a/config-service-factory/build.gradle.kts b/config-service-factory/build.gradle.kts index 61e23cd3..d5b73ba5 100644 --- 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 index d3d97f62..cdb8947d 100644 --- 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 index d2ca2df0..c96e5f16 100644 --- 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 responseObserver) { try { ConfigResourceContext configResourceContext = getConfigResourceContext(request); - Optional 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 = + Optional 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 index ffc94b48..72fc3551 100644 --- 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/config-service-impl/src/main/java/org/hypertrace/config/service/store/DocumentConfigStore.java b/config-service-impl/src/main/java/org/hypertrace/config/service/store/DocumentConfigStore.java index 6ee6063f..1310df48 100644 --- a/config-service-impl/src/main/java/org/hypertrace/config/service/store/DocumentConfigStore.java +++ b/config-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/config-service-impl/src/test/java/org/hypertrace/config/service/store/DocumentConfigStoreTest.java b/config-service-impl/src/test/java/org/hypertrace/config/service/store/DocumentConfigStoreTest.java index 9176b85c..a5e56d29 100644 --- a/config-service-impl/src/test/java/org/hypertrace/config/service/store/DocumentConfigStoreTest.java +++ b/config-service-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