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: Remove promise manager usage from ingestion warnings & db #17650

Merged
merged 1 commit into from
Oct 9, 2023

Conversation

tiina303
Copy link
Contributor

Problem

Promise manager adds confusion to how things work and could be causing the ingestion pipeline to stall at points, lets nuke it.

Changes

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

How did you test this code?

@tiina303 tiina303 changed the title chore: Remove promise manager usage from ingestion warnings chore: Remove promise manager usage from ingestion warnings & db Sep 27, 2023
@tiina303 tiina303 force-pushed the promise-manager-in-ingestion-warning branch from a5fb70e to c0e6164 Compare September 27, 2023 16:00
@tiina303 tiina303 marked this pull request as ready for review September 27, 2023 17:03
@tiina303 tiina303 requested a review from a team September 27, 2023 17:03
@bretthoerner
Copy link
Contributor

Interesting, I hadn't run into PromiseManager yet.

Behaviorally the diff makes sense to me, but it does seem like we'll now block and await a Kafka produce for every captureIngestionWarning call, rather than just fire-and-forget it in the background. Won't this only add to ingestion stalls?

@tiina303
Copy link
Contributor Author

Behaviorally the diff makes sense to me, but it does seem like we'll now block and await a Kafka produce for every captureIngestionWarning call, rather than just fire-and-forget it in the background. Won't this only add to ingestion stalls?

yes, potentially, but our ingestion parallelism should handle this, no? Also afaik there's two options: someone somewhere waits for it or we drop it occasionally. Wouldn't be surprised if we did the latter.

Copy link
Contributor

@xvello xvello left a comment

Choose a reason for hiding this comment

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

Yeah, let's nuke this manager 🔥

nit for possibly later: I'm wondering whether we really need the delivery guarantee though: awaiting ensures we never lose an ingestion warning on pod crash, but do we really need this guarantee? I feel like we could get away with firing off captureIngestionWarning and not awaiting it.

captureIngestionWarning returns once the message is in the rdkafka producer queue, that we expect to be properly flushed on graceful shutdown (we await kafkaProducer.disconnect() on closeHub), so the only data loss scenario is pod crash/oom.

@tiina303
Copy link
Contributor Author

tiina303 commented Oct 6, 2023

There was something about voided promises that was the reason, promiseManager was added in the first place iirc ... too many voided promises cause some crashes / stalling 🤷‍♀️

@tiina303 tiina303 force-pushed the promise-manager-in-ingestion-warning branch from c252fc0 to 86ddb3a Compare October 6, 2023 12:25
@tiina303 tiina303 merged commit a9eb17a into master Oct 9, 2023
69 checks passed
@tiina303 tiina303 deleted the promise-manager-in-ingestion-warning branch October 9, 2023 14:25
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