replace unmaintained fxhash by maintained rustc-hash#578
replace unmaintained fxhash by maintained rustc-hash#578ggirol-rc wants to merge 3 commits intotikv:masterfrom
Conversation
|
Welcome @ggirol-rc! It looks like this is your first PR to tikv/raft-rs 🎉 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaced fxhash with rustc-hash across manifests and type aliases, switched DefaultHashBuilder to use Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip You can disable poems in the walkthrough.Disable the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
harness/tests/integration_cases/test_raft.rs (1)
32-32: HashSet duplication is necessary due to the private type scope insrc/lib.rs.The
HashSettype alias insrc/lib.rsis private (not exported), so it cannot be imported by test code. This duplication intest_raft.rsis acceptable and necessary. However, if the maintainers decide to expose internal hashing infrastructure as part of the public API, consider re-exportingDefaultHashBuilderand the type aliases to ensure consistency across the codebase.
ggirol-rc
force-pushed
the
remove_unmaintained_fxhash
branch
from
d7a1993 to
d2980bf
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
harness/tests/integration_cases/test_raw_node.rs (2)
323-326: Consider sorting all order-sensitive fields, not justvoters.Only
votersis sorted before comparison, butConfStatealso haslearners,voters_outgoing, andlearners_nextfields that come fromHashSetiteration and could be non-deterministic. Current test data happens to use single-element or empty collections for those fields in this code path, so this works today — but it's fragile if test cases are added later.♻️ Suggested defensive sorting
let mut cs = cs.unwrap(); exp.voters.sort(); cs.voters.sort(); + exp.learners.sort(); + cs.learners.sort(); + exp.voters_outgoing.sort(); + cs.voters_outgoing.sort(); + exp.learners_next.sort(); + cs.learners_next.sort(); assert_eq!(exp, cs);
371-375: Same here — consider sorting all list fields for robustness.Only
learnersis sorted, butvoters,voters_outgoing, andlearners_nextinexp2/cscould also have non-deterministic ordering. Current test data is safe (single-element voters, empty outgoing/next), but a helper that normalizes all fields would be more maintainable.♻️ Suggested defensive sorting
let mut cs = raw_node.apply_conf_change(&leave_cc).unwrap(); cs.learners.sort(); + cs.voters.sort(); + cs.voters_outgoing.sort(); + cs.learners_next.sort(); let mut exp2 = exp2.unwrap(); exp2.learners.sort(); + exp2.voters.sort(); + exp2.voters_outgoing.sort(); + exp2.learners_next.sort(); assert_eq!(cs, exp2);
ggirol-rc
force-pushed
the
remove_unmaintained_fxhash
branch
from
d2980bf to
43a6807
Compare
ggirol-rc
force-pushed
the
remove_unmaintained_fxhash
branch
2 times, most recently
from
e144779 to
c3650fb
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
harness/tests/integration_cases/test_raw_node.rs (1)
323-326: Consider centralizingConfStatenormalization for assertions.Line 323–Line 326 and Line 371–Line 375 normalize different fields inline. A small helper would reduce repetition and avoid missing fields in future cases.
♻️ Suggested consolidation
+fn normalize_conf_state(cs: &mut ConfState) { + cs.voters.sort_unstable(); + cs.learners.sort_unstable(); + cs.voters_outgoing.sort_unstable(); + cs.learners_next.sort_unstable(); +} ... - let mut cs = cs.unwrap(); - exp.voters.sort(); - cs.voters.sort(); + let mut cs = cs.unwrap(); + normalize_conf_state(&mut exp); + normalize_conf_state(&mut cs); assert_eq!(exp, cs); ... - let mut cs = raw_node.apply_conf_change(&leave_cc).unwrap(); - cs.learners.sort(); + let mut cs = raw_node.apply_conf_change(&leave_cc).unwrap(); + normalize_conf_state(&mut cs); let mut exp2 = exp2.unwrap(); - exp2.learners.sort(); + normalize_conf_state(&mut exp2); assert_eq!(cs, exp2);Also applies to: 371-375
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@harness/tests/integration_cases/test_raw_node.rs` around lines 323 - 326, Tests repeatedly mutate ConfState before assertions by sorting fields (e.g., exp.voters.sort(); cs.voters.sort()); extract this into a small helper like normalize_confstate(conf: &mut ConfState) that sorts all comparable collections (voters, learners, maybe learners_next/outgoing if present) and call it for both exp and cs before assert_eq!, replacing the inline sorts in the blocks that reference exp and cs to avoid duplication and missing fields in future.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@harness/tests/integration_cases/test_raw_node.rs`:
- Around line 323-326: Tests repeatedly mutate ConfState before assertions by
sorting fields (e.g., exp.voters.sort(); cs.voters.sort()); extract this into a
small helper like normalize_confstate(conf: &mut ConfState) that sorts all
comparable collections (voters, learners, maybe learners_next/outgoing if
present) and call it for both exp and cs before assert_eq!, replacing the inline
sorts in the blocks that reference exp and cs to avoid duplication and missing
fields in future.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
Cargo.tomlharness/Cargo.tomlharness/tests/integration_cases/test_raft.rsharness/tests/integration_cases/test_raw_node.rssrc/lib.rssrc/quorum/majority.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- Cargo.toml
BusyJay
left a comment
BusyJay
left a comment
There was a problem hiding this comment.
I'm worried that changing hash method may change the output result order and potentially lead to inconsistency between replicas that runs old and new versions.
| use crate::test_util::*; | ||
|
|
||
| type HashSet<K> = std::collections::HashSet<K, std::hash::BuildHasherDefault<fxhash::FxHasher>>; | ||
| type HashSet<K> = std::collections::HashSet<K, std::hash::BuildHasherDefault<rustc_hash::FxHasher>>; |
There was a problem hiding this comment.
How about type HashSet<K> = rustc_hash::FxHashSet<K>?
src/quorum/majority.rs
Outdated
|
|
||
| impl std::fmt::Display for Configuration { | ||
| fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { | ||
| // tests assume Display is deterministic |
There was a problem hiding this comment.
Don't sort here, as no production code requires deterministic
There was a problem hiding this comment.
I moved the sorting code to a #[cfg(test)] codepath
ggirol-rc
force-pushed
the
remove_unmaintained_fxhash
branch
from
c3650fb to
8df7e72
Compare
fwiw I could have a successful leader election between one node running this MR and two nodes running raft-rs 0.7.0 |
see https://rustsec.org/advisories/RUSTSEC-2025-0057 Signed-off-by: Guillaume Girol <guillaume.girol@rubycat.eu>
Signed-off-by: Guillaume Girol <guillaume.girol@rubycat.eu>
tests assume that the order of elements is deterministic. Without this you can be unlucky and get a tests failure like: ``` assertion failed: `(left == right)`' left: `"voters=(2 3 1)&&(1) autoleave\n1: StateProbe match=0 next=0\n2: StateProbe match=0 next=1\n3: StateProbe match=0 next=1\n"` right: `"voters=(1 2 3)&&(1) autoleave\n1: StateProbe match=0 next=0\n2: StateProbe match=0 next=1\n3: StateProbe match=0 next=1\n"` Differences (-left|+right): -voters=(2 3 1)&&(1) autoleave +voters=(1 2 3)&&(1) autoleave 1: StateProbe match=0 next=0 2: StateProbe match=0 next=1 3: StateProbe match=0 next=1 ``` Signed-off-by: Guillaume Girol <guillaume.girol@rubycat.eu>
ggirol-rc
force-pushed
the
remove_unmaintained_fxhash
branch
from
8df7e72 to
8e4cef1
Compare
|
Is there anything actionable which can move this forward? |
2 participants
replace unmaintained fxhash by maintained rustc-hash
see https://rustsec.org/advisories/RUSTSEC-2025-0057
This uncovered that some tests were sensitive to the order of HashSets, so I made them deterministic.
Summary by CodeRabbit
Bug Fixes
Chores
Tests