bi-directional full sync #131
Conversation
| if fullSync { | ||
| // TODO: handle full sync | ||
| go func() { |
There was a problem hiding this comment.
This seems wasteful to me. If the sender applies the changes from the receiver and their checksums match, a bidirectional full-sync wouldn't be needed. Now what we've got is bi-directional full-syncs by default.
Also, the unbounded go-routine creation frightens me a bit. I know they're meant to be lightweight, but not handling this by throttling the amount of bi-directional full-syncs that we issue seems like something that would come back to haunt us, at scale.
There was a problem hiding this comment.
I don't think it will full-sync all the time: if checksums match, it will return false:
ringpop-go/swim/disseminator.go
Line 165 in 6e836b4
However, I agree with the second argument: we should throttle the number of goroutines we spawn here, possibly one.
There was a problem hiding this comment.
I think what Jeff means is that if the one way full sync fixes the membership, we don't need the reverse full sync
There was a problem hiding this comment.
I don't think of this as wasteful. Full syncs are necessary because less wasteful methods: the normal dissemination protocol, has failed. I'm okay if the emergency mechanism is very wasteful, yet effective. As of now, it's half as wasteful and half as effective.
In some sense the reverse full syncs are already throttled by the average of receiving 5 pings per second. If we can't trust that this is always the case, I agree that we should throttle the reverse full syncs. One at the time seems appropriate to me.
…tForConvergence Ws refactor calls disco provider wait for convergence
CorgiMan
force-pushed
the
ws-bidirectional-full-sync
branch
2 times, most recently
from
8375d8d
to
e32e826
Compare
CorgiMan
force-pushed
the
ws-bidirectional-full-sync
branch
from
c73d897
to
a63584b
Compare
No description provided.