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

onChange should update instead of sync #682

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

EugeneZ
Copy link
Contributor

@EugeneZ EugeneZ commented Aug 17, 2023

When a document changes, the Redis extension performs a full re-sync exchange with every single other server in the cluster. This is way more expensive than just sending an update.

@ralphiee22
Copy link
Collaborator

I don't think redis guarantees message delivery which makes me worry about this change.
If messages are dropped then there will be updates that are missed by other clients.
I think that is part of the safe option of sending the entire document each time that if a single update is missed, then on a subsequent sync it will still have the full change set.

@ralphiee22
Copy link
Collaborator

If we want to change the system to only emit single change events, then we probably need to re-architect to use something like redis-streams, where we can more reliably confirm that a client receives emitted messages

@EugeneZ
Copy link
Contributor Author

EugeneZ commented Aug 18, 2023

Good point, did not think about that. It seems that doing a full sync only partially helps, though. If there is a problem with receiving messages, it can affect a full sync, too, right? Definitely okay with closing this PR if y'all think it's worth the tradeoff, but it just seems counter to the idea of YJS efficiency.

@ralphiee22
Copy link
Collaborator

I think @janthurau could speak more to why this implementation was chosen

@janthurau
Copy link
Collaborator

Thanks for your PR @EugeneZ! Will check this out next week.

In general it seems better to send the update messages, I'm just not sure what happens on newly connected redis clients, or if that still works if an update message gets lost (as raised by the @ralphiee22). An option could be to implement a simple counter field. If the counter-1 doesn't match previously received update event, the server could request a full sync again.

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