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

chore: stop using piscina worker methods for runEventPipeline #17893

Closed
wants to merge 1 commit into from

Conversation

tiina303
Copy link
Contributor

Problem

We don't really need to use piscina here and it's creating unneccesary confusion reading the code

Changes

👉 Stay up-to-date with PostHog coding conventions for a smoother review.

How did you test this code?

@tiina303 tiina303 requested review from xvello and a team October 11, 2023 14:27
Copy link
Contributor

@bretthoerner bretthoerner left a comment

Choose a reason for hiding this comment

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

Nice, makes a lot of things easier to follow.

@@ -94,6 +95,7 @@ export class EventPipelineRunner {
}

async runEventPipeline(event: PipelineEvent): Promise<EventPipelineResult> {
this.originalEvent = event
Copy link
Contributor

Choose a reason for hiding this comment

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

It's confusing that we store it in the constructor and also here. Does just 1 or the other not work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right, forgot to remove it from here ... I added if before creating a function outside of the class that call locations can use

@bretthoerner
Copy link
Contributor

bretthoerner commented Oct 13, 2023

Rebased this on master so I could try it out on my PR.

For historical records: @xvello and I noticed in #17932 that the Piscana layer was doing funny things with our objects. What should have been an array of Promise objects was turned into an array of plain Objects... so either the promises were dropped or they were awaited, either one is scary.

We should merge #17932 (which includes the commit from this PR) on Monday so we don't risk a behavior/performance change on Friday.

@xvello
Copy link
Contributor

xvello commented Oct 16, 2023

Thanks for the investation Brett. @tiina303 please don't merge this PR by itself, we'll merge it as part of #17932 to avoid a regression: we'd fail to process messages that are too big for the destination topic.

@xvello xvello closed this Oct 16, 2023
@bretthoerner bretthoerner deleted the nuke-workermethods branch October 16, 2023 14:45
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.

3 participants