Skip to content
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

fix(ingestion): do not create or update person from $snapshot events #13048

Merged
merged 4 commits into from
Dec 1, 2022

Conversation

yakkomajuri
Copy link
Contributor

Had to really hold myself back not to get into a big refactor.

Essentially, almost every step of the pipeline now has special handling for $snapshot events, which tells us that they should be a whole pipeline altogether.

That requires:

  • Renaming the steps since the numbers indicating order will be irrelevant and we'll want to reuse steps across pipelines
  • Finally getting rid of onSnapshot so we don't have to pipe the events to the async handlers step
  • Writing the new pipeline. It will essentially be: prepareEvent --> createEvent. No person processing, no buffer, no async handlers

Was really about to do this but I'm definitely in no place to start another refactor now, so going the easiest route.

@yakkomajuri yakkomajuri changed the title fix(ingestion): do not create or update person from events fix(ingestion): do not create or update person from $snapshot events Nov 30, 2022
Copy link
Contributor

@tiina303 tiina303 left a comment

Choose a reason for hiding this comment

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

lgtm pending tests pass

// TODO: Have the snapshot events pipeline be completely separate
// from all other events
if (event.event === '$snapshot') {
return runner.nextStep('prepareEventStep', event, personContainer)
Copy link
Contributor

Choose a reason for hiding this comment

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

this is silly, we're doing this step just to do normalizeEvent but yup agreed fastest way

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I started this PR off by setting up a runSnapshotEventPipeline, but chances are I'd just leave more WIP for others

@yakkomajuri yakkomajuri merged commit aa89545 into master Dec 1, 2022
@yakkomajuri yakkomajuri deleted the snapshot-person branch December 1, 2022 13:37
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.

2 participants