apollo_network_benchmark: add ReversedSqmr variant to NetworkProtocol enum#11553
Conversation
|
There hasn't been any activity on this pull request recently, and in order to prioritize active work, it has been marked as stale. |
| } | ||
| NetworkProtocol::ReveresedSqmr => { | ||
| // TODO(AndrewL): Implement ReveresedSqmr protocol registration | ||
| todo!("ReveresedSqmr protocol registration not yet implemented") |
There was a problem hiding this comment.
New protocol option panics at runtime
Medium Severity
NetworkProtocol::ReveresedSqmr is exposed as a selectable CLI value in node_args.rs, but register_protocol_channels handles it with todo!(). Choosing reversed-sqmr causes an immediate panic during node initialization, so the newly advertised protocol mode is not runnable.
Additional Locations (1)
a86c6c8 to
b16cd92
Compare
2817aba to
a0c4d0e
Compare
b16cd92 to
55e1b2d
Compare
a0c4d0e to
a497a4a
Compare
55e1b2d to
8067a3e
Compare
8067a3e to
4015380
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
| Sqmr, | ||
| /// Use Reversed SQMR where receivers initiate requests to broadcasters | ||
| #[value(name = "reversed-sqmr")] | ||
| ReveresedSqmr, |
There was a problem hiding this comment.
Misspelled enum variant affects serde serialization name
Medium Severity
The variant ReveresedSqmr is misspelled (should be ReversedSqmr — the 'e' and 's' are transposed). Because NetworkProtocol derives Serialize and Deserialize, serde will use the misspelled name ReveresedSqmr as the serialized representation. The clap #[value(name = "reversed-sqmr")] and the doc comment both use the correct spelling, creating an inconsistency where the CLI accepts the right name but serde uses the wrong one.
Additional Locations (1)
04611b6 to
bc7549c
Compare
4015380 to
b78d523
Compare
bc7549c to
cf93d25
Compare
b78d523 to
50dae42
Compare



Note
Medium Risk
Low code churn, but it introduces a new user-selectable mode that will crash at runtime (
todo!()) if chosen.Overview
Adds a new
NetworkProtocol::ReveresedSqmrCLI-selectable variant (--network-protocol reversed-sqmr) for the network benchmark.Updates
register_protocol_channelsto handle this variant, but currently callstodo!()with a placeholder comment instead of registering channels, so selecting it will panic at runtime.Written by Cursor Bugbot for commit 50dae42. This will update automatically on new commits. Configure here.