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(recordings): use node-librdkafka for ingester production #14460

Merged
merged 14 commits into from
Apr 11, 2023

Conversation

hazzadous
Copy link
Contributor

@hazzadous hazzadous commented Feb 28, 2023

Previously we've been using the KafkaJS Producer with a wrapper around
it to handle batching. There are a number of issues with the batching
implementation e.g. not having a way to provide guarantees on delivery
and rather than fix that, we can simply use the librdkafka Producer
which is a lot more mature and battle-tested.

Ultimately we should probably move over to librdkafka for all the plugin server
operations, but I'm starting with recordings first and just the producer which is a
little less critical than the other workloads.

Problem

Changes

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

How did you test this code?

Previously we've been using the KafkaJS Producer with a wrapper around
it to handle batching. There are a number of issues with the batching
implementation e.g. not having a way to provide guarantees on delivery
and rather than fix that, we can simply use the librdkafka Producer
which is a lot more mature and battle-tested.
@hazzadous hazzadous marked this pull request as draft March 1, 2023 13:09
@mariusandra
Copy link
Collaborator

We switched away from librdkafka because it was flakey and caused the app to crash in unpredicatble ways :). @Twixes even had to submit a C++ patch to them.

Might be better now, and the early days of the plugin server were different (more async stuff flying around), but don't assume it's a silver bullet.

@hazzadous
Copy link
Contributor Author

@mariusandra do you happen to have a link to examples of issues you'd seen? I've only migrated the functional tests to use the librdkafka producer so far, the seems ok so far. But would love to fail fast here rather than move everything over.

Is it more than just the patch that @Twixes submitted?

@mariusandra
Copy link
Collaborator

I tried finding something the old archive, but couldn't. I also don't remember the specifics, but more of a general feeling of things being odd/sluggish/flakey/crashy whenever we used it.

Sorry, I know I'm not very helpful :(

@hazzadous
Copy link
Contributor Author

I tried finding something the old archive, but couldn't. I also don't remember the specifics, but more of a general feeling of things being odd/sluggish/flakey/crashy whenever we used it.

Sorry, I know I'm not very helpful :(

No it's all useful data. I'll have a look through that repo to see how it was being used.

@hazzadous hazzadous marked this pull request as ready for review March 1, 2023 18:46
@hazzadous hazzadous requested review from a team and benjackwhite March 1, 2023 18:48
@hazzadous hazzadous requested a review from Twixes March 1, 2023 19:14
@hazzadous hazzadous removed request for a team, benjackwhite and Twixes March 9, 2023 09:07
@posthog-bot
Copy link
Contributor

This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the stale label – otherwise this will be closed in another week.

@xvello xvello removed the stale label Mar 17, 2023
@posthog-bot
Copy link
Contributor

This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the stale label – otherwise this will be closed in another week.

@posthog-bot
Copy link
Contributor

This PR was closed due to lack of activity. Feel free to reopen if it's still relevant.

@posthog-bot posthog-bot closed this Apr 3, 2023
@xvello xvello reopened this Apr 3, 2023
@xvello xvello removed the stale label Apr 3, 2023
@hazzadous
Copy link
Contributor Author

@xvello running the tests with master merged in 🤞

@hazzadous hazzadous enabled auto-merge (squash) April 11, 2023 09:31
@hazzadous hazzadous disabled auto-merge April 11, 2023 09:38
@hazzadous hazzadous merged commit c349798 into master Apr 11, 2023
@hazzadous hazzadous deleted the chore/use-node-librdkafka branch April 11, 2023 15:44
xvello added a commit that referenced this pull request Apr 11, 2023
tiina303 pushed a commit that referenced this pull request Apr 11, 2023
#15032)

Revert "chore(recordings): use node-librdkafka for ingester production (#14460)"

This reverts commit c349798.
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