-
Notifications
You must be signed in to change notification settings - Fork 28
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
Implement priority broadcasts for local changes #152
Conversation
c0f1c07
to
48d3cd0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is trying to implement #148, I have to say that it does not fit the bill. What's described in that issue is related to broadcasts and not client input. It should've been as simple as: use a different enum variant or add a flag to broadcasted message to have them processed as a priority in the pipeline. It should've been added for all broadcasts going to ring0
members.
I think the simplest is to consider the broadcast a priority for a single hop.
In a way, all changes are pretty high priority, we want everything synced as fast as possible, but we have to choose more carefully.
The idea with ring0 directed broadcasts being higher priority is to make a "read after write" situation better from the same region. Concretely for Fly: an operation triggered from a gateway in LHR should be near-instantaneously replicated to all nodes in LHR because the next query from a client coming into LHR might reference the change that was just made, except the gateway might not even know about it if it forwarded a change to a worker node in the same region.
5a6e453
to
964a28c
Compare
52e1a81
to
d946950
Compare
d946950
to
33719aa
Compare
33719aa
to
769e166
Compare
I'll rebase this again once #134 is merged, just wanted to get the conflicts out the way |
769e166
to
3db082f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good!
Initial implementation of priority broadcasts and cleaning up the processing_changes function pipeline a bit. Some open questions in this implementation:
Are there/ what are better ways of exposing a "priority insert" over the API?
My rationale was to allow certain changes to "jump the queue" for several broadcast cycles if it is deemed important enough. It would be up to the caller to decide which insert is important. This does add a lot of overhead in the API though.
Should priority inserts still use a queue?
Currently every priority broadcast will immediately insert the received change (in an async task). When receiving a series of priority inserts, maybe it would make sense to collect a few (less than 20) together to make inserts more efficient.
I'll have a look at the CI failure. I had to update a few of the tests to make them pass again, because of the change to
Statement
. I would have expected the updatedFrom<&str>
implementation to take care of it but 🤷