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

Broadcast known message optimization #1

Open
DasIch opened this issue Apr 11, 2023 · 1 comment
Open

Broadcast known message optimization #1

DasIch opened this issue Apr 11, 2023 · 1 comment

Comments

@DasIch
Copy link

DasIch commented Apr 11, 2023

I hope it's ok, if I ask a question here. I'm wondering about the optimization introduced in this commit here: d96079d

I'm curious why this particular optimization was chosen. It seems to me there would be other ways of achieving this that are more obvious (to me at least) and would work better. Like acknowledging "gossip" messages with a "gossip_ok" messages and updating known when receiving a "gossip_ok" message. That would also allow for a couple of follow-up optimizations like not sending "gossip" messages, when all messages are known to the other node. You can further include messages in "gossip_ok" to gossip back.

Having said that I'm also new to distributed systems, so I suspect I might be missing something rather important and the above is a terrible idea for some reason. So I'd really appreciate to learn more about why you chose to optimize it in this way.

@jonhoo
Copy link
Owner

jonhoo commented Apr 23, 2023

I talked about this a little bit during the stream when I wrote that commit, but since then I've come around to (again) thinking we should have an explicit ACK to gossip messages. The main reason isn't so much to avoid sending messages though, but rather that the gossip topology isn't necessary symmetrical. That is, A may gossip to B, but B may never gossip to A. As a result, in the current implementation, A will never learn which messages B has received, and will thus likely continue to re-send old messages to B.

I'll likely end up fixing this the next time I do a stream on this :)

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

No branches or pull requests

2 participants