Set a better default for maxP and fix maxP value after bootstrap. #97
Conversation
|
It's worth noting that the if condition I removed doesn't appear in the node codebase. |
|
That's a useful comment, thanks 👍 |
| s.node.Bootstrap(&BootstrapOptions{ | ||
| Hosts: bootstrapList, | ||
| Stopped: true, | ||
| }) |
There was a problem hiding this comment.
Can we test here that maxP of s.node is not smaller than pFactor?
|
lgtm except the one comment. Have you tested with tickcluster etc? Are there effects on number of full syncs? I'll rebase your work on mine and test it on the cluster as soon as Ben's change has landed |
|
@CorgiMan Good question. Here's how the full syncs look now (each cell has the number of observed full syncs):
I'd say it's a good improvement over what we used to see before. |
|
How did you gather these metrics? Can you post the command/script? It would be interesting for all to see. |
|
@dansimau I've run tickcluster with different cluster sizes and counted the number of full sync log messages printed until the cluster converged. It's all manual. ☕ |
|
MANUAL!??? I'm disgusted. 😉 |
|
lgtm |
|
lgtm, before merging, let me deploy on ringpopd-go |
|
Thanks! I'll wait with the merge. |
|
Let's get the Merge! |
Two things going on:
maxPshould never be lower than 15, considering the math used to adjust it.maxPdoesn't reflect the ring size.