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: Flush sessions on partition revoke #17436

Merged
merged 22 commits into from
Sep 21, 2023
Merged

Conversation

benjackwhite
Copy link
Contributor

@benjackwhite benjackwhite commented Sep 14, 2023

Problem

We currently destroy sessions and restart from scratch when rebalancing. This is wasteful and results in new requests potentially failing as the data isn't available. Given that it is a relatively rare occurrence, it would be simpler to flush whatever we have if possible

Changes

  • Flush all related sessions for a partition on revoke
  • Make flush always return a promise so that we can easily await in progress flushes
  • Track the partitions we have assigned and lock them via redis
  • Wait to consume messages until we have claimed all assigned partitions
  • Handle case where a partition is re-assigned whilst we are revoking it
  • Ensure the lock is constant, i.e. we should periodically lock not just when we consume

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

How did you test this code?

Sorry, something went wrong.

@benjackwhite benjackwhite marked this pull request as ready for review September 18, 2023 14:39
Copy link
Member

@pauldambra pauldambra left a comment

Choose a reason for hiding this comment

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

passed through just reading the code

@benjackwhite benjackwhite merged commit b14be47 into master Sep 21, 2023
@benjackwhite benjackwhite deleted the feat/flush-on-revoke branch September 21, 2023 12:03
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.

None yet

2 participants