-
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
Aggrerate processor : add option to allow raw events #4598
Conversation
Signed-off-by: Krishna Kondaka <[email protected]>
Signed-off-by: Krishna Kondaka <[email protected]>
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.
Nice feature! I have a few comments about some of the terms and behavior.
@@ -32,6 +32,9 @@ public class AggregateProcessorConfig { | |||
@NotNull | |||
private Boolean localMode = false; | |||
|
|||
@JsonProperty("allow_raw_events") |
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 think this is a rather confusing name for a couple of reasons: 1) What is a "raw" event, and 2) allow indicates that something is permitted to be used.
We are adding a feature to also "output the input event." Perhaps: output_unaggregated_events
or output_original_events
.
@@ -116,11 +119,15 @@ public Collection<Record<Event>> doExecute(Collection<Record<Event>> records) { | |||
final Event aggregateActionResponseEvent = handleEventResponse.getEvent(); | |||
|
|||
if (aggregateActionResponseEvent != null) { | |||
aggregateActionResponseEvent.getMetadata().addTags(List.of(AGGREGATED_TAG)); |
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 may be a breaking behavior. Anybody who is currently outputting tags as a means to detect failures is now going to get tags that indicate normal events.
I think we should do one of the following instead: 1) Require the user to provide this tag; 2) Only include this when the user configured to send the unaggregated events; or 3) Tag the unaggregated events with an unaggregated
tag.
Signed-off-by: Krishna Kondaka <[email protected]>
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.
Nice, thanks for making this change!
@@ -95,6 +96,9 @@ public Collection<Record<Event>> doExecute(Collection<Record<Event>> records) { | |||
final List<Event> concludeGroupEvents = actionOutput != null ? actionOutput.getEvents() : null; | |||
if (!concludeGroupEvents.isEmpty()) { | |||
concludeGroupEvents.stream().forEach((event) -> { | |||
if (aggregatedEventsTag != null) { | |||
event.getMetadata().addTags(List.of(aggregatedEventsTag)); |
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 might be nice to add an addTag(String)
method to EventMetadata to simplify life for the callers, but this isn't critical.
Description
Aggrerate processor : add option to allow raw events.
Aggregate processor only sends aggregated events by default. This option allows sending the raw events in addition to the aggregated events. The aggregated events are tagged to be identify the events separately from the raw events.
Issues Resolved
Resolves #[Issue number to be closed when this PR is merged]
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.