-
Notifications
You must be signed in to change notification settings - Fork 207
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.
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
Load AggregateAction, implement AggregateAction.handleEvent in AggregateProcessor, add GroupStateManager and AggregateIdentificationKeysHasher classes for use in AggregateProcessor #931
Merged
graytaylor0
merged 3 commits into
opensearch-project:main
from
graytaylor0:AggregateHandleEvent
Jan 25, 2022
Merged
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
a871e28
Load AggregateAction, implement AggregateAction.handleEvent in Aggreg…
graytaylor0 1c62523
Add GroupState and IdentificationHash classes, address PR comments
graytaylor0 c5a7806
Rename GroupStateManager to AggregateGroupManager, rename GroupState …
graytaylor0 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
21 changes: 21 additions & 0 deletions
21
...ssor/src/main/java/com/amazon/dataprepper/plugins/processor/aggregate/AggregateGroup.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
/* | ||
* Copyright OpenSearch Contributors | ||
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
package com.amazon.dataprepper.plugins.processor.aggregate; | ||
|
||
import java.util.HashMap; | ||
import java.util.Map; | ||
|
||
public class AggregateGroup { | ||
private final Map<Object, Object> groupState = new HashMap<>(); | ||
|
||
AggregateGroup() { | ||
|
||
} | ||
|
||
public Map<Object, Object> getGroupState() { | ||
return groupState; | ||
} | ||
} |
31 changes: 31 additions & 0 deletions
31
...c/main/java/com/amazon/dataprepper/plugins/processor/aggregate/AggregateGroupManager.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
/* | ||
* Copyright OpenSearch Contributors | ||
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
package com.amazon.dataprepper.plugins.processor.aggregate; | ||
|
||
import java.util.Map; | ||
|
||
import com.google.common.collect.Maps; | ||
|
||
class AggregateGroupManager { | ||
|
||
private final Map<AggregateIdentificationKeysHasher.IdentificationHash, AggregateGroup> allGroupStates = Maps.newConcurrentMap(); | ||
|
||
AggregateGroupManager() { | ||
|
||
} | ||
|
||
AggregateGroup getAggregateGroup(final AggregateIdentificationKeysHasher.IdentificationHash identificationHash) { | ||
final AggregateGroup aggregateGroup = allGroupStates.get(identificationHash); | ||
|
||
if (aggregateGroup != null) { | ||
return aggregateGroup; | ||
} | ||
|
||
final AggregateGroup newAggregateGroup = new AggregateGroup(); | ||
allGroupStates.put(identificationHash, newAggregateGroup); | ||
return newAggregateGroup; | ||
} | ||
} |
45 changes: 45 additions & 0 deletions
45
...com/amazon/dataprepper/plugins/processor/aggregate/AggregateIdentificationKeysHasher.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,45 @@ | ||
/* | ||
* Copyright OpenSearch Contributors | ||
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
package com.amazon.dataprepper.plugins.processor.aggregate; | ||
|
||
import com.amazon.dataprepper.model.event.Event; | ||
|
||
import java.util.HashMap; | ||
import java.util.List; | ||
import java.util.Map; | ||
|
||
class AggregateIdentificationKeysHasher { | ||
private final List<String> identificationKeys; | ||
AggregateIdentificationKeysHasher(final List<String> identificationKeys) { | ||
this.identificationKeys = identificationKeys; | ||
} | ||
|
||
IdentificationHash createIdentificationKeyHashFromEvent(final Event event) { | ||
final Map<Object, Object> identificationKeysHash = new HashMap<>(); | ||
for (final String identificationKey : identificationKeys) { | ||
identificationKeysHash.put(identificationKey, event.get(identificationKey, Object.class)); | ||
} | ||
return new IdentificationHash(identificationKeysHash); | ||
} | ||
|
||
public static class IdentificationHash { | ||
private final Map<Object, Object> hash; | ||
|
||
IdentificationHash(final Map<Object, Object> hash) { | ||
this.hash = hash; | ||
} | ||
|
||
@Override | ||
public boolean equals(Object other) { | ||
return this.hash.equals(other); | ||
} | ||
|
||
@Override | ||
public int hashCode() { | ||
return hash.hashCode(); | ||
} | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
64 changes: 64 additions & 0 deletions
64
...st/java/com/amazon/dataprepper/plugins/processor/aggregate/AggregateGroupManagerTest.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,64 @@ | ||
/* | ||
* Copyright OpenSearch Contributors | ||
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
package com.amazon.dataprepper.plugins.processor.aggregate; | ||
|
||
import org.junit.jupiter.api.BeforeEach; | ||
import org.junit.jupiter.api.Test; | ||
|
||
import java.util.Collections; | ||
import java.util.HashMap; | ||
import java.util.Map; | ||
import java.util.UUID; | ||
|
||
import static org.hamcrest.CoreMatchers.notNullValue; | ||
import static org.hamcrest.MatcherAssert.assertThat; | ||
import static org.hamcrest.CoreMatchers.equalTo; | ||
import static org.hamcrest.Matchers.is; | ||
import static org.hamcrest.CoreMatchers.sameInstance; | ||
|
||
public class AggregateGroupManagerTest { | ||
|
||
private AggregateGroupManager aggregateGroupManager; | ||
|
||
private AggregateIdentificationKeysHasher.IdentificationHash identificationHash; | ||
|
||
@BeforeEach | ||
void setup() { | ||
final Map<Object, Object> identificationKeysHash = new HashMap<>(); | ||
identificationKeysHash.put(UUID.randomUUID().toString(), UUID.randomUUID().toString()); | ||
|
||
identificationHash = new AggregateIdentificationKeysHasher.IdentificationHash(identificationKeysHash); | ||
} | ||
|
||
private AggregateGroupManager createObjectUnderTest() { | ||
return new AggregateGroupManager(); | ||
} | ||
|
||
@Test | ||
void getGroupState_with_non_existing_group_state_creates_and_returns_new_group_state_and_adds_to_allGroupStates() { | ||
aggregateGroupManager = createObjectUnderTest(); | ||
|
||
final AggregateGroup emptyAggregateGroup = aggregateGroupManager.getAggregateGroup(identificationHash); | ||
assertThat(emptyAggregateGroup, notNullValue()); | ||
assertThat(emptyAggregateGroup.getGroupState(), equalTo(Collections.emptyMap())); | ||
|
||
final AggregateGroup secondAggregateGroup = aggregateGroupManager.getAggregateGroup(identificationHash); | ||
assertThat(secondAggregateGroup, notNullValue()); | ||
assertThat(secondAggregateGroup, is(sameInstance(emptyAggregateGroup))); | ||
} | ||
|
||
@Test | ||
void getGroupState_returns_a_mutable_GroupState_Map() { | ||
aggregateGroupManager = createObjectUnderTest(); | ||
|
||
final AggregateGroup firstAggregateGroup = aggregateGroupManager.getAggregateGroup(identificationHash); | ||
firstAggregateGroup.getGroupState().put(UUID.randomUUID().toString(), UUID.randomUUID().toString()); | ||
|
||
final AggregateGroup secondAggregateGroup = aggregateGroupManager.getAggregateGroup(identificationHash); | ||
assertThat(secondAggregateGroup, equalTo(firstAggregateGroup)); | ||
|
||
} | ||
} |
77 changes: 77 additions & 0 deletions
77
...amazon/dataprepper/plugins/processor/aggregate/AggregateIdentificationKeysHasherTest.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,77 @@ | ||
/* | ||
* Copyright OpenSearch Contributors | ||
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
package com.amazon.dataprepper.plugins.processor.aggregate; | ||
|
||
import com.amazon.dataprepper.model.event.Event; | ||
import com.amazon.dataprepper.model.event.JacksonEvent; | ||
import org.junit.jupiter.api.BeforeEach; | ||
import org.junit.jupiter.api.Test; | ||
|
||
import java.util.ArrayList; | ||
import java.util.HashMap; | ||
import java.util.List; | ||
import java.util.Map; | ||
import java.util.UUID; | ||
|
||
import static org.hamcrest.CoreMatchers.equalTo; | ||
import static org.hamcrest.MatcherAssert.assertThat; | ||
|
||
public class AggregateIdentificationKeysHasherTest { | ||
private Event event; | ||
private List<String> identificationKeys; | ||
private AggregateIdentificationKeysHasher aggregateIdentificationKeysHasher; | ||
|
||
@BeforeEach | ||
void setup() { | ||
identificationKeys = new ArrayList<>(); | ||
identificationKeys.add("firstIdentificationKey"); | ||
identificationKeys.add("secondIdentificationKey"); | ||
} | ||
|
||
private AggregateIdentificationKeysHasher createObjectUnderTest() { | ||
return new AggregateIdentificationKeysHasher(identificationKeys); | ||
} | ||
|
||
@Test | ||
void createIdentificationKeyHashFromEvent_returns_expected_Map() { | ||
aggregateIdentificationKeysHasher = createObjectUnderTest(); | ||
final Map<Object, Object> eventMap = new HashMap<>(); | ||
eventMap.put("firstIdentificationKey", UUID.randomUUID().toString()); | ||
eventMap.put("secondIdentificationKey", UUID.randomUUID().toString()); | ||
|
||
final Map<Object, Object> expectedResult = new HashMap<>(eventMap); | ||
|
||
eventMap.put(UUID.randomUUID().toString(), UUID.randomUUID().toString()); | ||
|
||
event = JacksonEvent.builder() | ||
.withEventType("event") | ||
.withData(eventMap) | ||
.build(); | ||
|
||
final AggregateIdentificationKeysHasher.IdentificationHash result = aggregateIdentificationKeysHasher.createIdentificationKeyHashFromEvent(event); | ||
assertThat(result, equalTo(expectedResult)); | ||
} | ||
|
||
@Test | ||
void createIdentificationKeysHashFromEvent_where_Event_does_not_contain_one_of_the_identification_keys_returns_expected_Map() { | ||
aggregateIdentificationKeysHasher = createObjectUnderTest(); | ||
final Map<Object, Object> eventMap = new HashMap<>(); | ||
eventMap.put("firstIdentificationKey", UUID.randomUUID().toString()); | ||
|
||
final Map<Object, Object> expectedResult = new HashMap<>(eventMap); | ||
expectedResult.put("secondIdentificationKey", null); | ||
|
||
eventMap.put(UUID.randomUUID().toString(), UUID.randomUUID().toString()); | ||
|
||
event = JacksonEvent.builder() | ||
.withEventType("event") | ||
.withData(eventMap) | ||
.build(); | ||
|
||
final AggregateIdentificationKeysHasher.IdentificationHash result = aggregateIdentificationKeysHasher.createIdentificationKeyHashFromEvent(event); | ||
assertThat(result, equalTo(expectedResult)); | ||
} | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am worried we have created a class for the groupState but are still exposing the underlying data structure and therefore coupling the data to the processor and action.
@dlvenable & @graytaylor0 , have we thought about providing methods / interface to alter the group state without exposing the underlying dats structure?
(I don't want to block this PR but I think this is good conversation to have given our community will be interacting with the state as they write aggregate actions.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a valid point. I have thought about it, and I just don't think we want to restrict the
AggregateAction
creators like that. It's going to be more work on their part, as instead of getting a javaMap
that they likely already understand and know how to interact with, they will get some Java class that we've made, that they will have to both read about to understand how to use, and may also find themselves limited in the control that they have over it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We definitely don't want to give them control of the
AggregateGroup
class, so the option is to extract the groupState map into aGroupState
class that wraps the Map. However, this doesn't seem to add much value as we would just be adding functions forput
,get
,putAll
, etc. that just call the actual Map functions, and there is nothing that I am aware of that we would really want to restrict the AggregateAction creator from calling on the Map. As far as validation goes, everything put into the Map will be turned into an Event, so the validation should happen there.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a one way door decision that will couple the group state to the map interface. Maybe not completely one way door but it would require a breaking change to extend group state beyond the map interface. I just want to clarify, I do not want to limit the creator of an Aggregate Action from updating the group state as the desire. My biggest concern in raising this point is extensibility.
I don't want to debate the design here and I think others across the group may be interested in this conversation. I will raise an issue for this.