refactor(hydro_test): migrate paxos cross_singleton to by_ref() singleton mechanism [ci-bench]#2983
Open
MingweiSamuel wants to merge 2 commits into
Open
refactor(hydro_test): migrate paxos cross_singleton to by_ref() singleton mechanism [ci-bench]#2983MingweiSamuel wants to merge 2 commits into
MingweiSamuel wants to merge 2 commits into
Conversation
Deploying hydro with
|
| Latest commit: |
0d4d8e0
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://db99e348.hydroflow.pages.dev |
| Branch Preview URL: | https://mingwei-paxos-ref-2.hydroflow.pages.dev |
…eton mechanism
Converted 6 out of 8 cross_singleton usages in paxos.rs to use the
by_ref() singleton reference mechanism:
- recommit_after_leader_election: 3 cross_singleton calls replaced with
by_ref() captures in filter_map and map closures (p_ballot and
p_p1b_max_checkpoint captured as refs)
- sequence_payload: 1 cross_singleton replaced with by_ref() in map
(p_ballot captured as ref)
- index_payloads (sliced!): 1 cross_singleton replaced with by_ref() in
map (base_slot captured as ref)
- acceptor_p2: 2 cross_singleton calls replaced with by_ref() captures
in filter_map and map closures (a_max_ballot captured as ref)
Left unchanged:
- acceptor_p1: 2 cross_singleton calls remain because the a_log parameter
comes from a forward_ref cycle, and by_ref() on cycle sources causes a
graph partitioning error ("Handoff succ not in subgraph")
Added + 'a lifetime bounds where required by by_ref() usage:
- PaxosLike::build, PaxosLike::with_client (paxos_with_client.rs)
- paxos_core, index_payloads (paxos.rs)
- compartmentalized_paxos_core, CoreCompartmentalizedPaxos::build
(compartmentalized_paxos.rs)
Updated IR snapshots (both stable and nightly) to reflect the new
reference nodes replacing cross_singleton patterns.
Co-authored-by: Infinity 🤖 <infinity@hydro.run>
PR: #2983
MingweiSamuel
changed the title
…_ref() The original PR #2903 left acceptor_p1's 2 cross_singleton calls unconverted because a_log came from a forward_ref cycle and caused a graph partitioning error. This issue has since been fixed in the framework, so both a_max_ballot and a_log can now use by_ref() successfully. - acceptor_p1: both cross_singleton(a_max_ballot) and cross_singleton(a_log) replaced with by_ref() captures in the map closure - Added + 'a bounds on L in acceptor_p1 and leader_election signatures All 8 cross_singleton usages in paxos.rs are now converted. Co-authored-by: Infinity 🤖 <infinity@hydro.run> PR: #2983
MingweiSamuel
force-pushed
the
mingwei/paxos-ref-2
branch
from
e417761 to
0d4d8e0
Compare
Contributor
📊 Benchmark Results✅ Benchmark completed! You can download the results from the links below. Run History:
Last updated: 2026-06-26T20:55:34.185Z |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors the Paxos dataflow construction in hydro_test to replace cross_singleton(...) usage patterns with the newer by_ref() singleton reference capture mechanism, and updates the IR snapshot outputs accordingly.
Changes:
- Migrates multiple Paxos pipeline joins from
cross_singletontoby_ref()captures inpaxos.rs(proposer and acceptor paths). - Adds
+ 'abounds to several generic parameters to satisfyby_ref()capture requirements. - Updates stable and nightly Mermaid IR snapshots to match the new reference nodes and resulting graph structure.
Reviewed changes
Copilot reviewed 5 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| hydro_test/src/cluster/paxos.rs | Replaces cross_singleton joins with by_ref() captures; adjusts generic bounds. |
| hydro_test/src/cluster/paxos_with_client.rs | Adds P: 'a bounds to the PaxosLike trait methods for by_ref() compatibility. |
| hydro_test/src/cluster/compartmentalized_paxos.rs | Adds P: 'a bounds to compartmentalized Paxos entrypoints. |
| hydro_test/src/cluster/snapshots/paxos_ir@proposer_mermaid.snap | Updates stable proposer IR snapshot after refactor. |
| hydro_test/src/cluster/snapshots/paxos_ir@acceptor_mermaid.snap | Updates stable acceptor IR snapshot after refactor. |
| hydro_test/src/cluster/snapshots-nightly/paxos_ir@acceptor_mermaid.snap | Updates nightly acceptor IR snapshot after refactor. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+498
to
+502
| .map(q!(|ballot| { | ||
| let max_ballot = a_max_ballot_ref.clone(); | ||
| let log = a_log_ref.clone(); | ||
| ( | ||
| ballot.clone(), | ||
| if ballot == max_ballot { | ||
| Ok(log) | ||
| } else { | ||
| Err(max_ballot) | ||
| } | ||
| ballot.proposer_id.clone(), |
Member
There was a problem hiding this comment.
Might be correct? Although the performance should be no worse than before.
shadaj
approved these changes
Jun 26, 2026
Comment on lines
+498
to
+502
| .map(q!(|ballot| { | ||
| let max_ballot = a_max_ballot_ref.clone(); | ||
| let log = a_log_ref.clone(); | ||
| ( | ||
| ballot.clone(), | ||
| if ballot == max_ballot { | ||
| Ok(log) | ||
| } else { | ||
| Err(max_ballot) | ||
| } | ||
| ballot.proposer_id.clone(), |
Member
There was a problem hiding this comment.
Might be correct? Although the performance should be no worse than before.
This file contains hidden or 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
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.
Converted 8 out of 8 cross_singleton usages in paxos.rs to use the
by_ref() singleton reference mechanism:
by_ref() captures in filter_map and map closures (p_ballot and
p_p1b_max_checkpoint captured as refs)
(p_ballot captured as ref)
map (base_slot captured as ref)
in filter_map and map closures (a_max_ballot captured as ref)
replaced with by_ref() captures in the map closure
Added + 'a lifetime bounds where required by by_ref() usage:
(compartmentalized_paxos.rs)
Updated IR snapshots (both stable and nightly) to reflect the new
reference nodes replacing cross_singleton patterns.