feat(rust): give the transport a retry policy, as a unit of its own - #732
wkirschenmann wants to merge 2 commits into
Conversation
☂️ Python Coverage
Overall Coverage
New FilesNo new covered files... Modified FilesNo covered modified files...
|
999d0bf to
90f020a
Compare
90f020a to
428134d
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 428134dbf7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /// What each wait is multiplied by; empty for the default. | ||
| #[serde(deserialize_with = "crate::config_utils::optional_parsed")] | ||
| #[cfg_attr(feature = "schema", schemars(with = "String"))] | ||
| back_off_multiplier: Option<f64>, |
There was a problem hiding this comment.
When GrpcClient__BackOffMultiplier is 0, negative, NaN, or infinite, optional_parsed accepts it because f64::from_str accepts these spellings. The resulting policy silently produces zero delays or, via unwrap_or(ceiling) in bounds(), jumps to the maximum delay instead of rejecting the invalid configuration, so an operator typo changes retry behavior unpredictably.
Useful? React with 👍 / 👎.
| pub use connect::{connect, https_connector, ConnectionError}; | ||
| pub use http2_config::Http2Config; | ||
| pub use proxy::{ProxyConfig, ProxyError, ProxySource}; | ||
| pub use retry_config::RetryConfig; |
There was a problem hiding this comment.
When downstream users depend on the main armonik crate, this new public configuration type is available only as armonik::transport::RetryConfig; the configuration facade in packages/rust/armonik/src/client/mod.rs:8-11 re-exports every other HttpConfig unit but omits this one. Code that needs to name or build a retry policy therefore cannot follow the existing armonik::client::* API and must reach into the transport namespace, so RetryConfig should be added to that re-export list.
Useful? React with 👍 / 👎.
4229ce5 to
b800c44
Compare
`RetryConfig` carries what ArmoniK's other clients hand grpc-dotnet, so a deployment behaves the same whichever client talks to it: five attempts, one second growing by 1.5 to a five-second ceiling, replaying on `Unavailable`, `Aborted` and `Unknown`. It is reached as `config.retry` and applied by whoever makes the calls, since a channel carries no notion of a call. Four options travel with the rest of the vocabulary, under no prefix: `MaxAttempts`, `InitialBackOff`, `MaxBackOff`, `BackOffMultiplier`. The unit declares plain fields and no names of its own, and `config.rs` states the prefix next to the field with `embed_prefixed!`, the way `tcp`, `http2` and `tls` are already stated, so the reading and the JSON schema both follow from that one declaration. The capital O is the PascalCase rendering of "back off", which `rename_all` produces from the field name alone; no option here carries a rename attribute. `InitialBackOff` and `MaxBackOff` are spelled as the C# client spells them, so a deployment setting `GrpcClient__InitialBackOff` reaches both. `BackOffMultiplier` differs from that client's `BackoffMultiplier` until the rename lands there; the README says so, and no alias covers it, since an alias would keep the misspelling alive on both sides. `RawRetry` holds each value in the type that states what it accepts, so a bad one is refused while the source's key is still known and no option name has to be written next to the check: `MaxAttempts` is a `NonZeroU32`, and `0`, which would mean one try and no replay exactly like `1`, is named by the document that spelled it. `config_utils` gains the generic reader serving it and the multiplier. The one rule spanning two options goes through `TryFrom<RawRetry>`, as `TlsConfig` does with `RawTls`: a `MaxBackOff` below `InitialBackOff` holds every wait down to the ceiling, so one of the two options does nothing and the source cannot say which was meant. Setting either alone reaches it, since the other keeps its default. `bounds()` multiplies through `Duration::try_from_secs_f64` rather than `mul_f64`, which panics on a negative, infinite or non-numeric multiplier; the fields are public, so such a value is reachable, and anything the constructor refuses settles on the ceiling instead. From packages/rust: cargo test -p armonik-transport --all-features, 90 passed 0 failed; cargo clippy -p armonik-transport --all-features --all-targets and cargo clippy -p armonik --all-features --all-targets, 0 warnings; cargo check -p armonik-transport --no-default-features, 0 warnings; cargo fmt --check and RUSTDOCFLAGS=-Dwarnings cargo doc, clean. cargo run -p armonik-transport --features schema --example generate_schema declares MaxAttempts, InitialBackOff, MaxBackOff and BackOffMultiplier, each as a string.
`f64` parses `nan`, `inf`, `0` and every negative as readily as a real multiplier, so a typo in `BackOffMultiplier` changed the schedule without a word: zero and below-one waits shrink instead of growing, and a value no `Duration` can hold falls to the ceiling, pinning every retry at the maximum. The reader now requires a finite number of at least 1, and names the option. One is allowed and is a policy: retry at a fixed interval. `bounds()` keeps its own guard. The fields are public, so a caller can still set a multiplier the reader would have refused, and a schedule that cannot be computed settles on the ceiling rather than panicking. `RetryConfig` joins the client facade's re-exports, so a caller names it the way it names every other unit rather than reaching into the transport crate. cargo test -p armonik-transport --all-features: 105 passed, 0 failed. cargo clippy -p armonik-transport -p armonik --all-features --all-targets: clean.
b800c44 to
f35c83a
Compare
|
|
❌ The last analysis has failed. |
1 participant
Give the transport a retry policy, configured as its own unit.
RetryConfigcarries what ArmoniK's other clients hand grpc-dotnet, so a deployment behaves thesame whichever client talks to it: five attempts, one second growing by 1.5 to a five-second
ceiling, replaying on
Unavailable,AbortedandUnknown. It is reached asconfig.retry, andapplied by whoever makes the calls: a channel carries no notion of a call, so nothing in
connectreads it.
Four options travel with the rest of the vocabulary, under no prefix:
MaxAttempts,InitialBackOff,MaxBackOff,BackOffMultiplier. The unit declares plain fields and no names ofits own;
config.rsstates the prefix next to the field withembed_prefixed!, as it already doesfor
tcp,http2andtls, so the reading and the JSON schema both come from that onedeclaration. The capital O is the PascalCase rendering of "back off", produced by
rename_allfromthe field name alone: no option here carries a rename attribute.
One name differs from the C# client, on purpose.
InitialBackOffandMaxBackOffare spelledexactly as
ArmoniK.Api.Client/Options/GrpcClient.csspells them, and both sides read the sameGrpcClient__prefix, so those two reach either client.BackOffMultiplierdoes not: C# spells itBackoffMultiplier, with a lowercase o. Until the rename lands there, a deployment settingGrpcClient__BackoffMultiplieris read by C# and not by this crate. No alias covers it, since analias would keep the misspelling alive on both sides; the README records the divergence.
RawRetryholds each value in the type that states what it accepts, so a bad one is refused whilethe source's key is still known and no option name has to be written next to the check:
MaxAttemptsis aNonZeroU32, and0- which would mean one try and no replay, exactly like1config_utilsgains the generic reader that serves itand the multiplier.
The one rule spanning two options goes through
TryFrom<RawRetry>, asTlsConfigdoes withRawTls: aMaxBackOffbelowInitialBackOffholds every wait down to the ceiling, so one of thetwo does nothing and the source cannot say which was meant. Setting either alone reaches it, since
the other keeps its default.
bounds()is the schedule those options describe, one item per replay and nothing past the lastattempt. The growth goes through
Duration::try_from_secs_f64rather thanmul_f64, which panicson a multiplier a caller set to a negative, infinite or non-numeric value; anything it refuses
settles on the ceiling.
Left for their own PRs, out of #707: the
retry!macro and theGrpcStatustrait, which are thecaller-facing loop, together with the
fastrandjitter draw only that loop consumes;MaxRetryBufferPerCallandMaxRetryUnarySize, which bound a replay buffer nothing here holds; andRetryableStatusCodesas an option, which no other ArmoniK client exposes - the three codes stay aprogrammatic field, fixed as C# fixes them.
10 tests. In
retry_config.rs: the defaults are the ones the other clients use; the bounds grow bythe multiplier and stop at the ceiling; there is one bound per replay and none beyond; a multiplier
no
Durationcan hold settles on the ceiling. Inconfig.rs, through a document: the optionsdefault; each one is read; a fractional multiplier survives the trip through text; zero attempts is
refused by the option rather than read as one; a ceiling below the initial back off is refused and
names both options; a ceiling equal to it is a constant wait rather than an error. The existing
an_empty_option_reads_as_its_defaultandthe_named_option_carries_the_prefix...cases gain thefour options, and
tests/schema.rspins them in the vocabulary and pinsRetryableStatusCodesoutof it.
From
packages/rust, all green:cargo test -p armonik-transport --all-features(90 passed, 0failed),
cargo clippy -p armonik-transport --all-features --all-targetsandcargo clippy -p armonik --all-features --all-targets(0 warnings),cargo check -p armonik-transport --no-default-features(0 warnings),cargo fmt --check.cargo run -p armonik-transport --features schema --example generate_schemalists the four options as strings.