-
Notifications
You must be signed in to change notification settings - Fork 83
fix(event processor): Flush current batch when event with different context received #341
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
Conversation
…ext, flush as soon as a different context is observed
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.
lgtm
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.
Looks good. Minor feedback about comments.
flushInterval: 100, | ||
maxQueueSize: 100, | ||
sink: sinkFn, | ||
batchComparator: (n1, n2) => (n1 % 2) === (n2 % 2) |
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.
nit. Can you add a comment about what this thing is doing? It will be useful for future maintainers.
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 changed it to a more straightforward test checking the first character of a string, and added comments.
const conversionEvent = createConversionEvent() | ||
const impressionEvent2 = createImpressionEvent() |
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.
nit. Can you add a note that createImpressionEvent
and createConversionEvent
create events with revision '1' and so line 300 makes sense with that context.
Or leave a comment for line 300 i.e. setting different revision to be able to test flush.
Summary
Prior to this change, event processor's buffer could contain events with different contexts. At flush time, the buffered events would be grouped by context and a request would be dispatched for each group.
With this change, we introduce an invariant: all events in the buffer share the same context. As soon as an event with different context is received, the queue is flushed. With this restriction, at most one request is dispatched per flush.
batchComparator
) is provided toDefaultEventQueue
. When enqueuing an event,batchComparator
is used to determine whether the incoming event can be included into the current batch. If not, we flush.LogTierV1EventProcessor
provides abatchComparator
when constructing aDefaultEventQueue
that checks equality of all context properties.AbstractEventProcessor
, the grouping step is removed from the flush process.Test plan
New unit tests. Manually tested.
Issues
OASIS-5160