[5/n] New domain-aware nodeset selector #2530
Conversation
AhmedSoliman
force-pushed
the
pr2530
branch
2 times, most recently
from
0f78cb7 to
b076f3c
Compare
AhmedSoliman
force-pushed
the
pr2530
branch
2 times, most recently
from
e988072 to
71e15fd
Compare
There was a problem hiding this comment.
Impressive work @AhmedSoliman. From what I can tell, the algorithm looks correct. I left my usual typo comments since I couldn't make any more meaningful contributions. +1 for merging :-)
| Self { | ||
| hashing_id: 0, | ||
| target_size: 0, | ||
| salt: 14712721395741015273, |
There was a problem hiding this comment.
Is this your favorite or a random number?
| /// to collect enough nodes that span the widest defined scope in replication property without | ||
| /// giving much attention to intermediate levels in the hierarchy. This approach is proven to be |
There was a problem hiding this comment.
So if we define {region: 2, zone: 5, nodes: 5} we don't guarantee that we will replicate across 5 zones, right?
There was a problem hiding this comment.
From nodeset generation perspective, no, but it's up to the spread selector and nodeset checker to enforce that. That said, this will do its absolute best to achieve this if it's possible within the current topology.
| .unwrap(); | ||
|
|
||
| assert!(nodeset.contains(1)); | ||
| assert_eq!(nodeset.len(), 6); |
There was a problem hiding this comment.
The nodeset size is 6 because we run the choose_domains twice (since all nodes are effectively writable), right?
There was a problem hiding this comment.
It's 6 because we try and get nodes from every domain possible to increase write availability.
| // zone-1 N3 | ||
| //- region1 | ||
| // zone-0 N1, N4 | ||
| // zone-1 N1 |
There was a problem hiding this comment.
Should N1 be in two zones?
Stack created with Sapling. Best reviewed with ReviewStack.