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(plugin-server): use rdkafka autocommit by storing our own offsets #17657

Merged
merged 3 commits into from
Oct 3, 2023

Conversation

bretthoerner
Copy link
Contributor

@bretthoerner bretthoerner commented Sep 27, 2023

Problem

When using the rdkafka consumer, we observe a ton of Broker: Specified group generation id is not valid errors that I believe are a race between our commits and rebalance callbacks.

See: https://docs.google.com/document/d/1ysRuQxFlCclXLLay3XBSOMHvWq5fHhqlrkkh9TdWthI/edit#heading=h.qt2rbhy3lwyu

Changes

Rather than doing an async commit ourselves, we store offsets in rdkafka and tell it to autocommit. It promises to do the right thing with respect to rebalances.

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

How did you test this code?

Locally, to ensure stored offsets are committed. And CI.

@bretthoerner bretthoerner requested a review from a team September 27, 2023 21:27
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.

LGTM, we'll need to double-check it does not break the recordings consumer before merging.

@bretthoerner bretthoerner merged commit 76396ab into master Oct 3, 2023
69 checks passed
@bretthoerner bretthoerner deleted the brett/store-offsets branch October 3, 2023 13:52
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