apollo_propeller: add tree topology core logic#11057
Conversation
60f745c to
59d480b
Compare
sirandreww-starkware
left a comment
There was a problem hiding this comment.
@sirandreww-starkware made 11 comments and resolved 4 discussions.
Reviewable status: 1 of 2 files reviewed, 8 unresolved discussions (waiting on @guy-starkware, @noamsp-starkware, and @ShahakShama).
crates/apollo_propeller/src/tree.rs line 23 at r1 (raw file):
Previously, ShahakShama wrote…
Rename Tree to Schedule or Router
Done.
crates/apollo_propeller/src/tree.rs line 25 at r1 (raw file):
Previously, ShahakShama wrote…
+1
Decided to go with a sorted vector for efficiency
crates/apollo_propeller/src/tree.rs line 25 at r1 (raw file):
Previously, ShahakShama wrote…
I thought this is a channel. Rename it to channel and consider adding a dedicated type / type alias
Done.
crates/apollo_propeller/src/tree.rs line 50 at r1 (raw file):
Previously, ShahakShama wrote…
Just because we wrote it on the board as f doesn't mean in the code it should be f as well. Rename to num_data_shards
Done.
crates/apollo_propeller/src/tree.rs line 53 at r1 (raw file):
Previously, ShahakShama wrote…
Why? I want propeller to work with 1 node as well
Added a TODO
crates/apollo_propeller/src/tree.rs line 69 at r1 (raw file):
Previously, ShahakShama wrote…
Add comment that we deduct the publisher
Done.
crates/apollo_propeller/src/tree.rs line 84 at r1 (raw file):
Previously, ShahakShama wrote…
cluster nodes?
Done.
crates/apollo_propeller/src/tree.rs line 86 at r1 (raw file):
Previously, ShahakShama wrote…
Rename to move_to_new_channel
I actually want to change this struct to be immutable after it is created. I want to do this because I want to tie this with the fact the same channel should not change the set inside of it once it is created. I moved this code to new. Please take another look
crates/apollo_propeller/src/tree.rs line 108 at r1 (raw file):
Previously, ShahakShama wrote…
+1
Done
crates/apollo_propeller/src/tree.rs line 162 at r1 (raw file):
Previously, guy-starkware wrote…
Seems superfluous.
Done
crates/apollo_propeller/src/tree.rs line 62 at r1 (raw file):
} pub fn calculate_data_shards(&self) -> usize {
Done
59d480b to
a5e50a1
Compare
e1c75d6 to
d0e51d1
Compare
guy-starkware
left a comment
There was a problem hiding this comment.
@guy-starkware reviewed 3 files and all commit messages, and resolved 1 discussion.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @noamsp-starkware and @ShahakShama).
ShahakShama
left a comment
There was a problem hiding this comment.
@ShahakShama reviewed 1 file and all commit messages, made 7 comments, and resolved 6 discussions.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @noamsp-starkware and @sirandreww-starkware).
crates/apollo_propeller/src/tree.rs line 23 at r1 (raw file):
Previously, sirandreww-starkware (Andrew Luka) wrote…
Done.
rename the file as well (or add a TODO)
crates/apollo_propeller/src/tree.rs line 25 at r2 (raw file):
pub struct PropellerScheduleManager { /// All nodes in the channel with their stake, sorted by (stake, peer_id) descending. channel_nodes: Vec<(PeerId, Stake)>,
I tend to think it's better to not store the Stake as long as we don't do anything with it
crates/apollo_propeller/src/tree.rs line 38 at r2 (raw file):
impl PropellerScheduleManager { /// Create a new propeller tree manager.
comment is outdated. I think it's unneccesary to begin with and can be deleted
crates/apollo_propeller/src/tree.rs line 60 at r2 (raw file):
let total_nodes = nodes.len(); // Ensure num_data_shards is at least 1 for small networks (N=2,3) // Standard formula: num_data_shards = floor((N-1)/3)
Add the comment here that you reduce N by 1 because you exclude the publisher
crates/apollo_propeller/src/tree_test.rs line 7 at r2 (raw file):
#[test] fn test_create_empty_tree_manager() {
Rename tree across this file too
crates/apollo_propeller/src/tree_test.rs line 21 at r2 (raw file):
#[test] fn test_update_nodes_and_f_calculation() {
rename f here
Also rename update_nodes
crates/apollo_propeller/src/tree_test.rs line 63 at r2 (raw file):
#[test] fn test_update_nodes_missing_local_peer() {
rename update_nodes to new
d0e51d1 to
76fe8e9
Compare
a5e50a1 to
628be45
Compare
sirandreww-starkware
left a comment
There was a problem hiding this comment.
@sirandreww-starkware made 7 comments.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @noamsp-starkware and @ShahakShama).
crates/apollo_propeller/src/tree.rs line 23 at r1 (raw file):
Previously, ShahakShama wrote…
rename the file as well (or add a TODO)
Done.
crates/apollo_propeller/src/tree.rs line 25 at r2 (raw file):
Previously, ShahakShama wrote…
I tend to think it's better to not store the Stake as long as we don't do anything with it
I would rather change this implementation to the "build = 1/3 stake" than go backwards here. Would you allow that in this PR?
crates/apollo_propeller/src/tree.rs line 38 at r2 (raw file):
Previously, ShahakShama wrote…
comment is outdated. I think it's unneccesary to begin with and can be deleted
Done.
crates/apollo_propeller/src/tree.rs line 60 at r2 (raw file):
Previously, ShahakShama wrote…
Add the comment here that you reduce N by 1 because you exclude the publisher
Done.
crates/apollo_propeller/src/tree_test.rs line 7 at r2 (raw file):
Previously, ShahakShama wrote…
Rename tree across this file too
Done.
crates/apollo_propeller/src/tree_test.rs line 21 at r2 (raw file):
Previously, ShahakShama wrote…
rename f here
Also rename update_nodes
Done.
crates/apollo_propeller/src/tree_test.rs line 63 at r2 (raw file):
Previously, ShahakShama wrote…
rename update_nodes to new
Done.
76fe8e9 to
c4ac8b9
Compare
628be45 to
a076411
Compare
ShahakShama
left a comment
There was a problem hiding this comment.
@ShahakShama reviewed 4 files and all commit messages, made 1 comment, and resolved 7 discussions.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @noamsp-starkware).
a076411 to
47c436e
Compare
c4ac8b9 to
430d3e9
Compare
ShahakShama
left a comment
There was a problem hiding this comment.
@ShahakShama reviewed 1 file and all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @noamsp-starkware).
430d3e9 to
ad7c4c3
Compare
47c436e to
af6baee
Compare
ad7c4c3 to
ec7cad8
Compare
af6baee to
4b7f792
Compare
sirandreww-starkware
left a comment
There was a problem hiding this comment.
@sirandreww-starkware reviewed 4 files and all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @noamsp-starkware).
83fcd09

No description provided.