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

improvement: implement a subscription notification batcher #217

Merged
merged 8 commits into from
Oct 7, 2024

Conversation

zachdaniel
Copy link
Contributor

@zachdaniel zachdaniel commented Oct 2, 2024

This is still a WIP, but by using this strategy we can allow actions to send subscription updates out-of-band.

What remains to be done:

  1. This on its own may already be useful, but what we really need to do is now leverage these batches and resolve the loads for lists of notifications. This will massively reduce the amount of queries required to dispatch data, and should significantly raise the scaling properties of these subscriptions.
  2. testing & benchmarks

Later improvements

We have batchers, but in the future what we may need is a dynamic amount of them, perhaps partitioned on topic, and load balanced across a cluster potentially. I think we need to start small and test the waters in production use cases with the one orchestrating batcher first, though.

@barnabasJ
Copy link
Contributor

@zachdaniel, should we merge this now? I can create a PR against main this week, removing the unnecessary data we copy.

@zachdaniel zachdaniel marked this pull request as ready for review October 7, 2024 11:21
@zachdaniel
Copy link
Contributor Author

Yep! Just need to fix conflicts

@barnabasJ
Copy link
Contributor

Yep! Just need to fix conflicts

I can do that, if you want me to.

@zachdaniel
Copy link
Contributor Author

That would be great, thank you 🙏 you're the best 😍

@barnabasJ barnabasJ added the enhancement New feature or request label Oct 7, 2024
@barnabasJ
Copy link
Contributor

@zachdaniel I rebased. However, I had to upgrade Ash because there was an error in the alias test. This also led to an igniter update. That now triggered some Igniter Dialyzer warnings.

It looks like those are just deprecation warnings. So I think it should be ok to merge this?

@zachdaniel
Copy link
Contributor Author

Yep, I'll address those, thanks!

@zachdaniel zachdaniel merged commit 3cb2c98 into main Oct 7, 2024
15 of 16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants