Skip to content

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

Merged
merged 11 commits into from
Aug 13, 2019

Conversation

mjc1283
Copy link
Contributor

@mjc1283 mjc1283 commented Aug 9, 2019

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.

  • A new comparator function (batchComparator) is provided to DefaultEventQueue. 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 a batchComparator when constructing a DefaultEventQueue that checks equality of all context properties.
  • In AbstractEventProcessor, the grouping step is removed from the flush process.

Test plan

New unit tests. Manually tested.

Issues

OASIS-5160

@coveralls
Copy link

coveralls commented Aug 9, 2019

Coverage Status

Coverage remained the same at 97.597% when pulling ae40538 on mcarroll/OASIS-5160 into 0ab1759 on master.

Copy link
Contributor

@mikeproeng37 mikeproeng37 left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Contributor

@aliabbasrizvi aliabbasrizvi left a 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)
Copy link
Contributor

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.

Copy link
Contributor Author

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()
Copy link
Contributor

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.

@mjc1283 mjc1283 merged commit a18a073 into master Aug 13, 2019
@mjc1283 mjc1283 deleted the mcarroll/OASIS-5160 branch August 13, 2019 16:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants