Skip to content

Commit

Permalink
fix the review comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@aman-bansal
aman-bansal committed Oct 12, 2023
1 parent adf217b commit c579760
Show file tree
Hide file tree
Showing 6 changed files with 64 additions and 47 deletions.
27 changes: 18 additions & 9 deletions config-service-impl/src/main/java/org/hypertrace/config/service/ConfigServiceGrpcImpl.java
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package org.hypertrace.config.service;

import static org.hypertrace.config.service.ConfigServiceUtils.emptyConfig;
import static org.hypertrace.config.service.ConfigServiceUtils.filterNull;
import static org.hypertrace.config.service.ConfigServiceUtils.merge;

Expand All @@ -10,6 +11,7 @@
import java.util.List;
import java.util.Map;
import java.util.Map.Entry;
import java.util.Optional;
import java.util.Set;
import java.util.stream.Collectors;
import lombok.extern.slf4j.Slf4j;
Expand Down Expand Up @@ -69,17 +71,24 @@ public void upsertConfig(
public void getConfig(
GetConfigRequest request, StreamObserver<GetConfigResponse> responseObserver) {
try {
ContextSpecificConfig config = configStore.getConfig(getConfigResourceContext(request));

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()));
}
// get the configs for the contexts mentioned in the request and merge them in the specified
// order
for (String context : request.getContextsList()) {
ContextSpecificConfig contextSpecificConfig =
Optional<ContextSpecificConfig> contextSpecificConfig =
configStore.getConfig(getConfigResourceContext(request, context));
config = merge(config, contextSpecificConfig);
if (contextSpecificConfig.isEmpty()) {
continue;
}
maybeConfig = Optional.of(merge(maybeConfig.get(), contextSpecificConfig.get()));
}

filterNull(config)
filterNull(maybeConfig.get())
.map(
nonNullConfig ->
GetConfigResponse.newBuilder()
Expand Down Expand Up @@ -124,18 +133,18 @@ public void deleteConfig(
DeleteConfigRequest request, StreamObserver<DeleteConfigResponse> responseObserver) {
try {
ConfigResourceContext configResourceContext = getConfigResourceContext(request);
ContextSpecificConfig configToDelete = configStore.getConfig(configResourceContext);
Optional<ContextSpecificConfig> maybeConfig = configStore.getConfig(configResourceContext);
// if configToDelete is null/empty (i.e. config value doesn't exist or is already deleted),
// then throw NOT_FOUND exception
if (configToDelete == null || ConfigServiceUtils.isNull(configToDelete.getConfig())) {
if (maybeConfig.isEmpty()) {
responseObserver.onError(Status.NOT_FOUND.asException());
return;
}

// delete the config for the specified config resource.
configStore.deleteConfigs(Set.of(configResourceContext));
responseObserver.onNext(
DeleteConfigResponse.newBuilder().setDeletedConfig(configToDelete).build());
DeleteConfigResponse.newBuilder().setDeletedConfig(maybeConfig.get()).build());
responseObserver.onCompleted();
} catch (Exception e) {
log.error("Delete config failed for request:{}", request, e);
Expand All @@ -160,7 +169,7 @@ public void deleteConfigs(
.map(this::getConfigResourceContext)
.collect(Collectors.toUnmodifiableSet());
Map<ConfigResourceContext, ContextSpecificConfig> configs =
configStore.getAllContextConfigs(configResourceContexts);
configStore.getContextConfigs(configResourceContexts);
// delete the configs for the specified config resources.
configStore.deleteConfigs(configResourceContexts);
responseObserver.onNext(
Expand Down
1 change: 1 addition & 0 deletions config-service-impl/src/main/java/org/hypertrace/config/service/ConfigServiceUtils.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ private ConfigServiceUtils() {
public static ContextSpecificConfig merge(
ContextSpecificConfig defaultContextSpecificConfig,
ContextSpecificConfig overridingContextSpecificConfig) {

Value defaultConfig = defaultContextSpecificConfig.getConfig();
Value overridingConfig = overridingContextSpecificConfig.getConfig();
if (isNull(defaultConfig)) {
Expand Down
12 changes: 7 additions & 5 deletions config-service-impl/src/main/java/org/hypertrace/config/service/store/ConfigStore.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,10 @@
import com.google.protobuf.Value;
import com.typesafe.config.Config;
import java.io.IOException;
import java.util.Collection;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.Optional;
import org.hypertrace.config.service.ConfigResource;
import org.hypertrace.config.service.ConfigResourceContext;
import org.hypertrace.config.service.v1.ContextSpecificConfig;
Expand Down Expand Up @@ -41,7 +42,8 @@ UpsertedConfig writeConfig(
* @param configResourceContext
* @return
*/
ContextSpecificConfig getConfig(ConfigResourceContext configResourceContext) throws IOException;
Optional<ContextSpecificConfig> getConfig(ConfigResourceContext configResourceContext)
throws IOException;

/**
* Get all the configs with the latest version(along with the context to which it applies) for the
Expand All @@ -51,8 +53,8 @@ UpsertedConfig writeConfig(
* @return the configs
* @throws IOException
*/
Map<ConfigResourceContext, ContextSpecificConfig> getAllContextConfigs(
Set<ConfigResourceContext> configResourceContexts) throws IOException;
Map<ConfigResourceContext, ContextSpecificConfig> getContextConfigs(
Collection<ConfigResourceContext> configResourceContexts) throws IOException;

/**
* Get all the configs with the latest version(along with the context to which it applies) for the
Expand Down Expand Up @@ -80,7 +82,7 @@ List<UpsertedConfig> writeAllConfigs(
*
* @param configResourceContexts
*/
void deleteConfigs(Set<ConfigResourceContext> configResourceContexts) throws IOException;
void deleteConfigs(Collection<ConfigResourceContext> configResourceContexts) throws IOException;

/**
* Health check for the backend store
Expand Down
29 changes: 14 additions & 15 deletions ...g-service-impl/src/main/java/org/hypertrace/config/service/store/DocumentConfigStore.java
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package org.hypertrace.config.service.store;

import static com.google.common.collect.Streams.zip;
import static org.hypertrace.config.service.ConfigServiceUtils.emptyConfig;
import static org.hypertrace.config.service.store.ConfigDocument.CONTEXT_FIELD_NAME;
import static org.hypertrace.config.service.store.ConfigDocument.RESOURCE_FIELD_NAME;
import static org.hypertrace.config.service.store.ConfigDocument.RESOURCE_NAMESPACE_FIELD_NAME;
Expand Down Expand Up @@ -160,36 +159,32 @@ public List<UpsertedConfig> writeAllConfigs(
}

@Override
public void deleteConfigs(Set<ConfigResourceContext> configResourceContexts) {
public void deleteConfigs(java.util.Collection<ConfigResourceContext> configResourceContexts) {
if (configResourceContexts.isEmpty()) {
return;
}
collection.delete(buildConfigResourceContextsFilter(configResourceContexts));
}

@Override
public ContextSpecificConfig getConfig(ConfigResourceContext configResourceContext)
public Optional<ContextSpecificConfig> getConfig(ConfigResourceContext configResourceContext)
throws IOException {
return getLatestVersionConfigDoc(configResourceContext)
.flatMap(this::convertToContextSpecificConfig)
.orElseGet(() -> emptyConfig(configResourceContext.getContext()));
.flatMap(this::convertToContextSpecificConfig);
}

@Override
public Map<ConfigResourceContext, ContextSpecificConfig> getAllContextConfigs(
Set<ConfigResourceContext> configResourceContexts) throws IOException {
public Map<ConfigResourceContext, ContextSpecificConfig> getContextConfigs(
java.util.Collection<ConfigResourceContext> configResourceContexts) throws IOException {
Map<ConfigResourceContext, Optional<ConfigDocument>> configDocs =
getLatestVersionConfigDocs(configResourceContexts);

return configDocs.entrySet().stream()
.filter(entry -> entry.getValue().isPresent())
.collect(
Collectors.toUnmodifiableMap(
Entry::getKey,
entry ->
entry
.getValue()
.flatMap(this::convertToContextSpecificConfig)
.orElseGet(() -> emptyConfig(entry.getKey().getContext()))));
entry.getValue().flatMap(this::convertToContextSpecificConfig).orElseThrow()));
}

@Override
Expand Down Expand Up @@ -238,7 +233,7 @@ private Optional<ConfigDocument> getLatestVersionConfigDoc(
}

private Map<ConfigResourceContext, Optional<ConfigDocument>> getLatestVersionConfigDocs(
Set<ConfigResourceContext> configResourceContexts) throws IOException {
java.util.Collection<ConfigResourceContext> configResourceContexts) throws IOException {
if (configResourceContexts.isEmpty()) {
return Collections.emptyMap();
}
Expand Down Expand Up @@ -269,7 +264,7 @@ private Map<ConfigResourceContext, Optional<ConfigDocument>> getLatestVersionCon
}

private Filter buildConfigResourceContextsFilter(
Set<ConfigResourceContext> configResourceContexts) {
java.util.Collection<ConfigResourceContext> configResourceContexts) {
List<Filter> childFilters =
configResourceContexts.stream()
.map(this::getConfigResourceFieldContextFilter)
Expand All @@ -280,7 +275,11 @@ private Filter buildConfigResourceContextsFilter(
Filter tenantIdFilter =
Filter.eq(
TENANT_ID_FIELD_NAME,
configResourceContexts.iterator().next().getConfigResource().getTenantId());
configResourceContexts.stream()
.findFirst()
.orElseThrow()
.getConfigResource()
.getTenantId());
return tenantIdFilter.and(configResourceFieldContextFilter);
}

Expand Down
35 changes: 20 additions & 15 deletions ...g-service-impl/src/test/java/org/hypertrace/config/service/ConfigServiceGrpcImplTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import java.util.Arrays;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.Set;
import org.hypertrace.config.service.store.ConfigStore;
import org.hypertrace.config.service.v1.ContextSpecificConfig;
Expand Down Expand Up @@ -106,19 +107,21 @@ void getConfig() throws IOException {
ConfigStore configStore = mock(ConfigStore.class);
when(configStore.getConfig(eq(configResourceWithoutContext)))
.thenReturn(
ContextSpecificConfig.newBuilder()
.setConfig(config1)
.setCreationTimestamp(10L)
.setUpdateTimestamp(12L)
.build());
Optional.of(
ContextSpecificConfig.newBuilder()
.setConfig(config1)
.setCreationTimestamp(10L)
.setUpdateTimestamp(12L)
.build()));
when(configStore.getConfig(eq(configResourceWithContext)))
.thenReturn(
ContextSpecificConfig.newBuilder()
.setConfig(config2)
.setContext(CONTEXT1)
.setCreationTimestamp(15L)
.setUpdateTimestamp(25L)
.build());
Optional.of(
ContextSpecificConfig.newBuilder()
.setConfig(config2)
.setContext(CONTEXT1)
.setCreationTimestamp(15L)
.setUpdateTimestamp(25L)
.build()));
ConfigServiceGrpcImpl configServiceGrpc = new ConfigServiceGrpcImpl(configStore);
StreamObserver<GetConfigResponse> responseObserver = mock(StreamObserver.class);

Expand Down Expand Up @@ -186,7 +189,8 @@ void deleteConfig() throws IOException {
ConfigStore configStore = mock(ConfigStore.class);
ContextSpecificConfig deletedConfig =
ContextSpecificConfig.newBuilder().setConfig(config2).setContext(CONTEXT1).build();
when(configStore.getConfig(eq(configResourceWithContext))).thenReturn(deletedConfig);
when(configStore.getConfig(eq(configResourceWithContext)))
.thenReturn(Optional.of(deletedConfig));
ConfigServiceGrpcImpl configServiceGrpc = new ConfigServiceGrpcImpl(configStore);
StreamObserver<DeleteConfigResponse> responseObserver = mock(StreamObserver.class);

Expand Down Expand Up @@ -222,7 +226,7 @@ void deleteConfigs() throws IOException {
getConfigResourceContext(context2),
emptyValue());

when(configStore.getAllContextConfigs(writeAllConfigsRequest.keySet()))
when(configStore.getContextConfigs(writeAllConfigsRequest.keySet()))
.thenReturn(
Map.of(
getConfigResourceContext(context1),
Expand Down Expand Up @@ -259,7 +263,8 @@ void deleteDefaultContextConfig() throws IOException {
ConfigStore configStore = mock(ConfigStore.class);
ContextSpecificConfig deletedConfig =
ContextSpecificConfig.newBuilder().setConfig(config2).build();
when(configStore.getConfig(eq(configResourceWithoutContext))).thenReturn(deletedConfig);
when(configStore.getConfig(eq(configResourceWithoutContext)))
.thenReturn(Optional.of(deletedConfig));
ConfigServiceGrpcImpl configServiceGrpc = new ConfigServiceGrpcImpl(configStore);
StreamObserver<DeleteConfigResponse> responseObserver = mock(StreamObserver.class);

Expand All @@ -280,7 +285,7 @@ void deleteDefaultContextConfig() throws IOException {
void deletingNonExistingConfigShouldThrowError() throws IOException {
ConfigStore configStore = mock(ConfigStore.class);
ContextSpecificConfig emptyConfig = ConfigServiceUtils.emptyConfig(CONTEXT1);
when(configStore.getConfig(eq(configResourceWithContext))).thenReturn(emptyConfig);
when(configStore.getConfig(eq(configResourceWithContext))).thenReturn(Optional.of(emptyConfig));
ConfigServiceGrpcImpl configServiceGrpc = new ConfigServiceGrpcImpl(configStore);
StreamObserver<DeleteConfigResponse> responseObserver = mock(StreamObserver.class);

Expand Down
7 changes: 4 additions & 3 deletions ...rvice-impl/src/test/java/org/hypertrace/config/service/store/DocumentConfigStoreTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import java.util.Iterator;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.Set;
import org.hypertrace.config.service.ConfigResource;
import org.hypertrace.config.service.ConfigResourceContext;
Expand Down Expand Up @@ -123,7 +124,7 @@ void getConfigs() throws IOException {
.setUpdateTimestamp(TIMESTAMP2)
.build();
Map<ConfigResourceContext, ContextSpecificConfig> actualConfigs =
configStore.getAllContextConfigs(Set.of(configResourceContext));
configStore.getContextConfigs(Set.of(configResourceContext));
assertEquals(Map.of(configResourceContext, expectedConfig), actualConfigs);
}

Expand All @@ -142,8 +143,8 @@ void getConfig() throws IOException {
.setCreationTimestamp(TIMESTAMP1)
.setUpdateTimestamp(TIMESTAMP2)
.build();
ContextSpecificConfig actualConfig = configStore.getConfig(configResourceContext);
assertEquals(expectedConfig, actualConfig);
Optional<ContextSpecificConfig> actualConfig = configStore.getConfig(configResourceContext);
assertEquals(Optional.of(expectedConfig), actualConfig);
}

@Test
Expand Down

0 comments on commit c579760

Please sign in to comment.