Skip to content

Extract notification methods in identified object store #77

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 12 commits into from
Nov 3, 2021
Merged

Extract notification methods in identified object store #77

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
a4ffb4d
extract notification methods
SrikarMannepalli Oct 27, 2021
d668ead
refactor
SrikarMannepalli Oct 27, 2021
7bbfd25
address review comments
SrikarMannepalli Oct 27, 2021
e03da88
address review comments
SrikarMannepalli Oct 28, 2021
fc095c4
update DefaultObjectStore and ContextuallyIdentifiedObjectStore
SrikarMannepalli Oct 28, 2021
98b9a87
address review comments
SrikarMannepalli Oct 28, 2021
2dc45c7
addres review comments
SrikarMannepalli Oct 29, 2021
92b5028
Merge branch 'main' into extract-notification-methods-in-identifiedOb…
SrikarMannepalli Oct 29, 2021
ce53ffe
address review comments
SrikarMannepalli Nov 2, 2021
211845f
remove unused object
SrikarMannepalli Nov 2, 2021
448a5dc
address review comments
SrikarMannepalli Nov 2, 2021
0a22430
add tests
SrikarMannepalli Nov 2, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions object-store/build.gradle.kts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ dependencies {
api(libs.hypertrace.grpcutils.context)

implementation(projects.configServiceChangeEventGenerator)
implementation(libs.slf4j.api)

annotationProcessor(libs.lombok)
compileOnly(libs.lombok)
Expand Down
18 changes: 18 additions & 0 deletions ...re/src/main/java/org/hypertrace/config/objectstore/ContextuallyIdentifiedObjectStore.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,14 @@ protected ContextuallyIdentifiedObjectStore(

protected abstract Value buildValueFromData(T data);

protected Value buildValueForChangeEvent(T data) {
return this.buildValueFromData(data);
}

protected String buildClassNameForChangeEvent(T data) {
return data.getClass().getName();
}

protected abstract String getConfigContextFromRequestContext(RequestContext requestContext);

private IdentifiedObjectStore<T> buildObjectStoreForContext(RequestContext context) {
Expand Down Expand Up @@ -86,6 +94,16 @@ protected Value buildValueFromData(T data) {
return ContextuallyIdentifiedObjectStore.this.buildValueFromData(data);
}

@Override
protected Value buildValueForChangeEvent(T data) {
return ContextuallyIdentifiedObjectStore.this.buildValueForChangeEvent(data);
}

@Override
protected String buildClassNameForChangeEvent(T data) {
return ContextuallyIdentifiedObjectStore.this.buildClassNameForChangeEvent(data);
}

@Override
protected String getContextFromData(T data) {
return ContextuallyIdentifiedObjectStore.this.getConfigContextFromRequestContext(
Expand Down
32 changes: 27 additions & 5 deletions object-store/src/main/java/org/hypertrace/config/objectstore/DefaultObjectStore.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import com.google.protobuf.Value;
import io.grpc.Status;
import java.util.Optional;
import lombok.extern.slf4j.Slf4j;
import org.hypertrace.config.service.change.event.api.ConfigChangeEventGenerator;
import org.hypertrace.config.service.v1.ConfigServiceGrpc.ConfigServiceBlockingStub;
import org.hypertrace.config.service.v1.ContextSpecificConfig;
Expand All @@ -18,6 +19,7 @@
*
* @param <T>
*/
@Slf4j
public abstract class DefaultObjectStore<T> {
private final ConfigServiceBlockingStub configServiceBlockingStub;
private final String resourceNamespace;
Expand Down Expand Up @@ -49,6 +51,14 @@ protected DefaultObjectStore(

protected abstract Value buildValueFromData(T data);

protected Value buildValueForChangeEvent(T data) {
return this.buildValueFromData(data);
}

protected String buildClassNameForChangeEvent(T data) {
return data.getClass().getName();
}

public Optional<T> getData(RequestContext context) {
try {
Value value =
Expand Down Expand Up @@ -91,12 +101,22 @@ public ConfigObject<T> upsertObject(RequestContext context, T data) {
if (response.hasPrevConfig()) {
configChangeEventGenerator.sendUpdateNotification(
context,
upsertedObject.getData().getClass().getName(),
response.getPrevConfig(),
response.getConfig());
this.buildClassNameForChangeEvent(upsertedObject.getData()),
this.buildDataFromValue(response.getPrevConfig())
.map(this::buildValueForChangeEvent)
.orElseGet(
() -> {
log.error(
"Unable to convert previousValue back to data for change event. Falling back to raw value {}",
response.getPrevConfig());
return response.getPrevConfig();
Copy link
Contributor

@saxenakshitiz saxenakshitiz Nov 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should not have this special case. Please note consumer will not be able to handle it.

Copy link
Contributor

@aaron-steinfeld aaron-steinfeld Nov 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consumer may or may not be able to, but at least we give them the option. This maps to the existing behavior (where we put in prevConfig as is, and don't check if it's transformable), but certainly open to other ideas (see #77 (comment) for various options I came up with)

}),
this.buildValueForChangeEvent(upsertedObject.getData()));
} else {
configChangeEventGenerator.sendCreateNotification(
context, upsertedObject.getData().getClass().getName(), response.getConfig());
context,
this.buildClassNameForChangeEvent(upsertedObject.getData()),
this.buildValueForChangeEvent(upsertedObject.getData()));
}
});
return upsertedObject;
Expand All @@ -120,7 +140,9 @@ public Optional<ConfigObject<T>> deleteObject(RequestContext context) {
configChangeEventGeneratorOptional.ifPresent(
configChangeEventGenerator ->
configChangeEventGenerator.sendDeleteNotification(
context, object.getData().getClass().getName(), deletedConfig.getConfig()));
context,
this.buildClassNameForChangeEvent(object.getData()),
this.buildValueForChangeEvent(object.getData())));
return Optional.of(object);
} catch (Exception exception) {
if (Status.fromThrowable(exception).equals(Status.NOT_FOUND)) {
Expand Down
35 changes: 29 additions & 6 deletions object-store/src/main/java/org/hypertrace/config/objectstore/IdentifiedObjectStore.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import java.util.List;
import java.util.Optional;
import java.util.stream.Collectors;
import lombok.extern.slf4j.Slf4j;
import org.hypertrace.config.service.change.event.api.ConfigChangeEventGenerator;
import org.hypertrace.config.service.v1.ConfigServiceGrpc.ConfigServiceBlockingStub;
import org.hypertrace.config.service.v1.ContextSpecificConfig;
Expand All @@ -24,6 +25,7 @@
*
* @param <T>
*/
@Slf4j
public abstract class IdentifiedObjectStore<T> {
private final ConfigServiceBlockingStub configServiceBlockingStub;
private final String resourceNamespace;
Expand Down Expand Up @@ -57,6 +59,14 @@ protected IdentifiedObjectStore(

protected abstract String getContextFromData(T data);

protected Value buildValueForChangeEvent(T data) {
return this.buildValueFromData(data);
}

protected String buildClassNameForChangeEvent(T data) {
Copy link
Contributor

@aaron-steinfeld aaron-steinfeld Oct 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add tests for the case where this is a different class than T?

Copy link
Contributor Author

@SrikarMannepalli SrikarMannepalli Oct 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not quite sure I understand. obj.getClass().getName() work for any class right? What am I missing?

Copy link
Contributor

@aaron-steinfeld aaron-steinfeld Oct 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably poorly phrased ask on my part. It does, but the reason we introduced buildClassNameForChangeEvent is that sometimes T is not the class we care about. I was interested in a test where we've overridden the default behavior (for example where T is MyInternalObject and we want our event to be sending MyApiObject)

Copy link
Contributor Author

@SrikarMannepalli SrikarMannepalli Nov 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. Could you give an example as to how to move forward 😬.

Copy link
Contributor

@aaron-steinfeld aaron-steinfeld Nov 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't a complete example, but you'd want to exercise the functionality we've added - with a separate api and persistence class. The tests should work as is with that change, but they also should be extended to verify that the mock change event writer is being called with the expected messages.

image

return data.getClass().getName();
}

protected List<ContextualConfigObject<T>> orderFetchedObjects(
List<ContextualConfigObject<T>> objects) {
return objects;
Expand Down Expand Up @@ -141,7 +151,10 @@ public Optional<ContextualConfigObject<T>> deleteObject(RequestContext context,
configChangeEventGeneratorOptional.ifPresent(
configChangeEventGenerator ->
configChangeEventGenerator.sendDeleteNotification(
context, object.getData().getClass().getName(), id, deletedConfig.getConfig()));
context,
this.buildClassNameForChangeEvent(object.getData()),
id,
this.buildValueForChangeEvent(object.getData())));
return Optional.of(object);
} catch (Exception exception) {
if (Status.fromThrowable(exception).equals(Status.NOT_FOUND)) {
Expand Down Expand Up @@ -181,6 +194,8 @@ private Optional<ContextualConfigObject<T>> processUpsertResult(
Optional<ContextualConfigObject<T>> optionalResult =
ContextualConfigObjectImpl.tryBuild(
response, this::buildDataFromValue, this::getContextFromData);

System.out.println(optionalResult);
optionalResult.ifPresent(
result -> {
if (response.hasPrevConfig()) {
Expand Down Expand Up @@ -213,9 +228,9 @@ private void tryReportCreation(RequestContext requestContext, ContextualConfigOb
configChangeEventGenerator ->
configChangeEventGenerator.sendCreateNotification(
requestContext,
result.getData().getClass().getName(),
this.buildClassNameForChangeEvent(result.getData()),
result.getContext(),
this.buildValueFromData(result.getData())));
this.buildValueForChangeEvent(result.getData())));
}

private void tryReportUpdate(
Expand All @@ -224,9 +239,17 @@ private void tryReportUpdate(
configChangeEventGenerator ->
configChangeEventGenerator.sendUpdateNotification(
requestContext,
result.getData().getClass().getName(),
this.buildClassNameForChangeEvent(result.getData()),
result.getContext(),
previousValue,
this.buildValueFromData(result.getData())));
this.buildDataFromValue(previousValue)
.map(this::buildValueForChangeEvent)
.orElseGet(
() -> {
log.error(
"Unable to convert previousValue back to data for change event. Falling back to raw value {}",
previousValue);
return previousValue;
}),
this.buildValueForChangeEvent(result.getData())));
}
}
61 changes: 50 additions & 11 deletions object-store/src/test/java/org/hypertrace/config/objectstore/DefaultObjectStoreTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,13 @@

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;

import com.google.protobuf.Struct;
import com.google.protobuf.Value;
import com.google.protobuf.util.Values;
import io.grpc.Status;
Expand Down Expand Up @@ -43,7 +45,7 @@ class DefaultObjectStoreTest {
@Mock(answer = Answers.CALLS_REAL_METHODS)
RequestContext mockRequestContext;

DefaultObjectStore<TestObject> store;
DefaultObjectStore<TestInternalObject> store;

@BeforeEach
void beforeEach() {
Expand All @@ -56,7 +58,8 @@ void generatesConfigReadRequestForGet() {
when(this.mockStub.getConfig(any()))
.thenReturn(GetConfigResponse.newBuilder().setConfig(Values.of("test")).build());

assertEquals(Optional.of(new TestObject("test")), this.store.getData(this.mockRequestContext));
assertEquals(
Optional.of(new TestInternalObject("test")), this.store.getData(this.mockRequestContext));

verify(this.mockStub, times(1))
.getConfig(
Expand Down Expand Up @@ -91,7 +94,7 @@ void generatesConfigDeleteRequest() {
assertEquals(
Optional.of(
new ConfigObjectImpl<>(
new TestObject("test"), TEST_CREATE_TIMESTAMP, TEST_UPDATE_TIMESTAMP)),
new TestInternalObject("test"), TEST_CREATE_TIMESTAMP, TEST_UPDATE_TIMESTAMP)),
this.store.deleteObject(mockRequestContext));

verify(this.mockStub)
Expand All @@ -104,6 +107,15 @@ void generatesConfigDeleteRequest() {
when(this.mockStub.deleteConfig(any())).thenThrow(Status.NOT_FOUND.asRuntimeException());

assertEquals(Optional.empty(), this.store.deleteObject(mockRequestContext));

verify(this.configChangeEventGenerator, times(1))
.sendDeleteNotification(
eq(this.mockRequestContext),
eq(TestApiObject.class.getName()),
eq(
Value.newBuilder()
.setStructValue(Struct.newBuilder().putFields("api_name", Values.of("test")))
.build()));
}

@Test
Expand All @@ -115,38 +127,65 @@ void generatesConfigUpsertRequest() {
.setUpdateTimestamp(TEST_UPDATE_TIMESTAMP.toEpochMilli())
.setConfig(Values.of("updated"))
.build());
assertEquals(
ConfigObject configObject =
new ConfigObjectImpl<>(
new TestObject("updated"), TEST_CREATE_TIMESTAMP, TEST_UPDATE_TIMESTAMP),
this.store.upsertObject(this.mockRequestContext, new TestObject("updated")));
new TestInternalObject("updated"), TEST_CREATE_TIMESTAMP, TEST_UPDATE_TIMESTAMP);
assertEquals(
configObject,
this.store.upsertObject(this.mockRequestContext, new TestInternalObject("updated")));
verify(this.mockStub, times(1))
.upsertConfig(
UpsertConfigRequest.newBuilder()
.setResourceName(TEST_RESOURCE_NAME)
.setResourceNamespace(TEST_RESOURCE_NAMESPACE)
.setConfig(Values.of("updated"))
.build());
verify(this.configChangeEventGenerator, times(1))
.sendCreateNotification(
eq(this.mockRequestContext),
eq(TestApiObject.class.getName()),
eq(
Value.newBuilder()
.setStructValue(Struct.newBuilder().putFields("api_name", Values.of("updated")))
.build()));
}

@lombok.Value
private static class TestObject {
private static class TestInternalObject {
Copy link
Contributor

@saxenakshitiz saxenakshitiz Nov 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit : The test seems to suggest that object persisted in DB is completely different from object being published as part of change event. That is certainly possible. However in most of our usecases, the object being persisted will encapsulate the object being published as part of change event.

String name;
}

private static class TestObjectStore extends DefaultObjectStore<TestObject> {
@lombok.Value
private static class TestApiObject {
String api_name;
}

private static class TestObjectStore extends DefaultObjectStore<TestInternalObject> {
private TestObjectStore(
ConfigServiceBlockingStub stub, ConfigChangeEventGenerator configChangeEventGenerator) {
super(stub, TEST_RESOURCE_NAMESPACE, TEST_RESOURCE_NAME, configChangeEventGenerator);
}

@Override
protected Optional<TestObject> buildDataFromValue(Value value) {
return Optional.of(new TestObject(value.getStringValue()));
protected Optional<TestInternalObject> buildDataFromValue(Value value) {
return Optional.of(new TestInternalObject(value.getStringValue()));
}

@Override
protected Value buildValueFromData(TestObject object) {
protected Value buildValueFromData(TestInternalObject object) {
return Values.of(object.getName());
}

@Override
protected Value buildValueForChangeEvent(TestInternalObject object) {
return Value.newBuilder()
.setStructValue(Struct.newBuilder().putFields("api_name", Values.of(object.getName())))
.build();
}

@Override
protected String buildClassNameForChangeEvent(TestInternalObject object) {
return TestApiObject.class.getName();
}
}
}
Loading