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

feat: add replay overflow limiter to rust capture #24803

Merged
merged 7 commits into from
Sep 10, 2024
Merged

Conversation

frankh
Copy link
Contributor

@frankh frankh commented Sep 5, 2024

Problem

replay overflow works the same way the billing limiter works - we check a redis key to see if a session_id is in the overflow key, and if it is we redirect it to another topic

to be honest i don't like that we have any of this logic in the sink, i think we should be calling the sink with topic+partition_key as arguments instead of figuring them out in the sink, but perhaps that's better for a later refactor

Changes

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

Does this work well for both Cloud and self-hosted?

How did you test this code?

added test

Copy link
Contributor

@benjackwhite benjackwhite left a comment

Choose a reason for hiding this comment

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

As far as I understand seems to be how it should work but definitely not the authority here

@oliverb123
Copy link
Contributor

I think this chunk of the code is a bit messy (generally, not due to this PR) - taking an action to refactor our kafka producer logic and possibly break it out into common, because we're gonna want to use it in other places soon too. For now, lgtm, send it (cautiously etc etc).

@frankh frankh force-pushed the frank/replay-overflow branch from 73d6eb0 to fba05f8 Compare September 9, 2024 14:44
@frankh frankh merged commit b8c4b42 into master Sep 10, 2024
77 checks passed
@frankh frankh deleted the frank/replay-overflow branch September 10, 2024 09:38
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