test: Spawn the next leader first #425
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bacv
reviewed
Sep 20, 2023
| // all nodes that will be subsequently spawned. | ||
| // If not, the leader will miss votes from nodes spawned before itself. | ||
| // This issue will be resolved by devising the block catch-up mechanism in the future | ||
| let mut nodes = vec![Self::spawn(configs.swap_remove(next_leader_idx)).await]; | ||
| let listening_addr = nodes[0].get_listening_address().await; |
There was a problem hiding this comment.
Shouldn't this be nodes[next_leader_idx].get_listening_address() if we want next leader to be available to other nodes before spawning (as initial_peer)?
There was a problem hiding this comment.
Since the nodes vector has only one element at line 211~212, we should use nodes[0] (which is the next leader spawned at line 211). But yeah, it's little not intuitive. Let me refactor this now.
There was a problem hiding this comment.
Okay, now I see, we append to the nodes array.
bacv
approved these changes
Sep 20, 2023
| // all nodes that will be subsequently spawned. | ||
| // If not, the leader will miss votes from nodes spawned before itself. | ||
| // This issue will be resolved by devising the block catch-up mechanism in the future | ||
| let mut nodes = vec![Self::spawn(configs.swap_remove(next_leader_idx)).await]; | ||
| let listening_addr = nodes[0].get_listening_address().await; |
There was a problem hiding this comment.
Okay, now I see, we append to the nodes array.
al8n
approved these changes
Sep 20, 2023
zeegomo
approved these changes
Sep 20, 2023
Merged
bacv
added a commit
that referenced
this pull request
Oct 5, 2023
* Use tree overlay in nomos node * Use tree overlay in tests module * Handle unexpected consensus vote stream end in tally * Spawn the next leader first (#425) * Report unhappy blocks in happy path test (#430) * report unhappy blocks in the happy path test * Make threshold configurable for TreeOverlay (#426) * Modified test, so that all nodes don’t all connect to the first node only (#442) * merge fix --------- Co-authored-by: Youngjoon Lee <taxihighway@gmail.com> Co-authored-by: Al Liu <scygliu1@gmail.com>
bacv
added a commit
that referenced
this pull request
Oct 5, 2023
* Use tree overlay in nomos node * Use tree overlay in tests module * Handle unexpected consensus vote stream end in tally * Add defaults for retry and delay in mixnet client * Add missing entries in config.yaml * Spawn the next leader first (#425) --------- Co-authored-by: Youngjoon Lee <taxihighway@gmail.com> Co-authored-by: Al Liu <scygliu1@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Solves the second bullet of #415 (comment).
Still the
ten_nodes_happywon't pass, like below. To resolve this, I'm going to open another PR.