Azure / Azure/azure-sdk-for-rust
Cosmos: Migrate `PartitionFailoverOptions` to `#[derive(CosmosOptions)]`
- Dominant language
- Rust
- Stars
- 884
- Forks
- 365
- Avg merge
- 2d 19h
- Merged PRs (30d)
- 112
Description
# Migrate `PartitionFailoverOptions` to `#[derive(CosmosOptions)]`
## Context
Follow-up from PR
[#4562](https://github.com/Azure/azure-sdk-for-rust/pull/4562) review.
The goal is to bring `PartitionFailoverOptions` in line with the rest of the
Cosmos options family (which now use the `CosmosOptions` derive for their
env-var plumbing) so env handling is generated and consistent instead of
hand-written.
## Relationship to #4655 / #4654 (read first)
This refactor is **conceptually orthogonal** to the PPCB env-var bug
([#4654](https://github.com/Azure/azure-sdk-for-rust/issues/4654)) and its fix
([#4655](https://github.com/Azure/azure-sdk-for-rust/pull/4655)), but it is
**not implementation-independent** — they touch the same file and the same
method, so they must be **sequenced**, not done in parallel.
**This refactor does NOT fix (and must not regress) the bug.** The #4654 bug is
that `DriverOptionsBuilder::build()` filled an omitted
`PartitionFailoverOptions` via `unwrap_or_default()`, and
`PartitionFailoverOptions::default()` hard-codes `circuit_breaker_enabled: true`
and reads no environment — so `AZURE_COSMOS_PPCB_*` was ignored on the common
"didn't pass options" path. That is a *consumer/chokepoint* bug, not a problem
with how `PartitionFailoverOptions` reads env. This migration changes *how the
env is read* (hand-written `parse_*` -> macro `from_env`); the bug is that the
env-reading path was never *invoked* on the omitted path. Migrating to
`#[derive(CosmosOptions)]` with `#[options(env_only)]` keeps `Default` pure
(env-free) by design, so on its own it would leave PPCB `true` on the omitted
path — i.e. it would **not** implicitly fix #4654.
**Sequencing: land #4655 first, then do this on top of it.** Expect overlap on:
- **Same method.** #4655 renames
`PartitionFailoverOptionsBuilder::build()` ->
`build_from_env(get_env: &dyn Fn(&str) -> Option)` and makes the
`DriverOptionsBuilder` chokepoint resolve PPCB from the env when options are
omitted (infallible / fail-soft to defaults on out-of-bounds). This refactor
rewrites the *body* of that same method to call the macro's `from_env`.
- **Same helpers.** #4655 adds an injectable `get_env` accessor to the three
`parse_*_from_env` helpers in `env_parsing.rs`; this refactor's goal is to
*delete* those `parse_*_from_env` calls. **Synergy:** the `CosmosOptions`
macro already provides closure-injected env reading via
`from_env_vars(|key| ...)`, so this refactor can *replace* #4655's bespoke
`get_env` plumbing with the macro's built-in mechanism (retiring the hand-
rolled accessor where it becomes dead).
- **Preserve #4655's behavior.** After #4655, the omitted-options path resolves
PPCB from the env at the `DriverOptionsBuilder::build()` chokepoint. This
refactor must keep that behavior intact (still env-resolved when omitted,
still fail-soft, `Default` still pure/env-free).
The "Current state" below describes the **pre-#4655** code. After #4655 lands,
re-baseline against `build_from_env(get_env)` and the chokepoint env resolution.
`PartitionFailoverOptions` and its builder
(`sdk/cosmos/azure_data_cosmos_driver/src/options/partition_failover.rs`) read
their environment variables with the hand-written helpers in
`src/options/env_parsing.rs`:
- `parse_from_env(...)` with a `ValidationBounds` argument
- `parse_duration_millis_from_env(...)` with explicit `(min_ms, max_ms)` bounds
- `parse_optional_bool_from_env(...)` for the incident kill switch
`PartitionFailoverOptionsBuilder::build()` is **fallible**
(`crate::error::Result`) and resolves each field as
`builder value -> env value -> compile-time default`, applying bounds
validation along the way. (After #4655 this method is
`build_from_env(get_env)` and the `DriverOptionsBuilder` chokepoint resolves
PPCB from the env when options are omitted — see "Relationship to #4655 / #4654"
above.)
The eight env-driven fields and their current bounds:
| Field | Env var | Default | Bounds |
|---|---|---|---|
| `circuit_breaker_enabled` | `AZURE_COSMOS_PPCB_ENABLED` | `true` | none |
| `circuit_breaker_enabled_override` | `AZURE_COSMOS_PPCB_ENABLED_OVERRIDE` | `None` | lenient bool; unrecognized = ignored |
| `read_failure_threshold` | `AZURE_COSMOS_PPCB_READ_FAILURE_THRESHOLD` | `10` | min `1` |
| `write_failure_threshold` | `AZURE_COSMOS_PPCB_WRITE_FAILURE_THRESHOLD` | `5` | min `1` |
| `counter_reset_window` | `AZURE_COSMOS_PPCB_COUNTER_RESET_WINDOW_MS` | `300_000 ms` | min `1_000 ms` |
| `partition_unavailability_duration` | `AZURE_COSMOS_PPCB_PARTITION_UNAVAILABILITY_DURATION_MS` | `5_000 ms` | min `1_000 ms` |
| `failback_sweep_interval` | `AZURE_COSMOS_PPCB_FAILBACK_SWEEP_INTERVAL_MS` | `300_000 ms` | min `1_000 ms` |
| `consecutive_hedge_win_threshold` | `AZURE_COSMOS_PPCB_CONSECUTIVE_HEDGE_WIN_THRESHOLD` | `5` | min `1` |
Note: `circuit_breaker_enabled_override` is **internal-only** (operator-facing
env kill switch). Its builder setter was removed and the getter is
`pub(crate)` (PR #4562). The migration must preserve that — the override stays
env-only and off the public builder/struct surface.
## What the `CosmosOptions` derive already supports
After PR #4562 the macro can cover most of this shape:
- `#[options(env_only)]` — generates only `from_env()` / `from_env_vars()` (no
View / Builder / `Default`), so an existing builder-style type can double as
its own env source.
- `#[option(env = "...")]` — env var per `Option` field via `FromStr`.
- `#[option(env = "...", parser = path::to::fn)]` — custom
`fn(&str) -> Option` parsing for field types without a suitable `FromStr`
(e.g. a `Duration` from a millisecond count). A `None` result is logged and
ignored (lenient).
- `#[option(env = "...", overridable)]` — recognizes a `{ENV}_OVERRIDE`
kill-switch variable and generates `from_env_override()` plus a top-priority
override layer.
## The gap: bounds validation
The `CosmosOptions` derive does **not** currently express per-field bounds
(min/max). `PartitionFailoverOptions` today rejects out-of-range values
(thresholds `>= 1`, durations `>= 1000 ms`) by returning `Err(...)` from
`build()`. A naive migration to `env_only` would silently drop bounds
validation, which is a behavior regression.
This is the main design decision the implementer must make. Options:
1. **Keep bounds validation in `build()`, move only env *reading* to the
macro.** Use `#[options(env_only)]` on an internal all-`Option` env
struct (or on the builder itself) to generate the env load, then keep the
explicit bounds checks + default resolution in
`PartitionFailoverOptionsBuilder::build()`. Smallest change; macro stays
simple; validation stays where it is. (Recommended starting point.)
2. **Extend the macro with a bounds attribute** (e.g.
`#[option(env = "...", min = 1)]` / `min_ms` / `max`) that generates the
range check and surfaces a typed error. Larger change to
`azure_data_cosmos_macros`, but removes the last bit of hand-written
plumbing and benefits other option groups. Could be its own sub-task.
Whichever path is chosen, the migration must not change observable behavior:
same env var names, same defaults, same bounds, same lenient-bool semantics for
the override, same fallible `build()` contract.
## Proposed approach (option 1, minimal)
- Introduce a `#[derive(CosmosOptions)] #[options(env_only)]` env-source struct
(all `Option` fields) covering the eight env vars, using `parser =`
for the three `Duration` fields and `overridable` for the kill switch.
- Have `PartitionFailoverOptionsBuilder::build()` call the generated
`from_env()` and then apply the existing default-resolution + bounds
validation (reusing `ValidationBounds` / the millisecond bounds) before
constructing the final `PartitionFailoverOptions`.
- Delete the now-unused direct calls to `parse_from_env` /
`parse_duration_millis_from_env` / `parse_optional_bool_from_env` from
`partition_failover.rs` (and any helper that becomes dead).
## Acceptance criteria
- [ ] `PartitionFailoverOptions` env handling is generated via
`#[derive(CosmosOptions)]` rather than hand-written `parse_*_from_env`
calls.
- [ ] All eight env vars keep their exact names, defaults, and bounds.
- [ ] Bounds violations still produce an error from `build()` (no silent
acceptance of out-of-range values).
- [ ] `AZURE_COSMOS_PPCB_ENABLED_OVERRIDE` stays env-only and internal: no
public builder setter, getter remains `pub(crate)`, lenient-bool
(unrecognized = ignored) semantics preserved.
- [ ] `circuit_breaker_enabled` default stays `true` (PPCB on by default).
- [ ] The omitted-options path still resolves PPCB from the environment per
#4655 (do not regress #4654): env-resolved when
`with_partition_failover_options` is not called, fail-soft on out-of-bounds,
and `Default` stays pure/env-free.
- [ ] Existing unit tests in `partition_failover.rs` pass unchanged (defaults,
custom round-trip, zero/below-min rejection, override precedence); add
coverage for any new generated path.
- [ ] No public API change to `PartitionFailoverOptions` /
`PartitionFailoverOptionsBuilder` (this is an internal refactor).
- [ ] If the macro is extended (option 2), add `azure_data_cosmos_macros`
tests + CHANGELOG entry for the new attribute.
## Notes / risk
- Pure internal refactor; no behavior or public-surface change intended. Keep
the diff observably equivalent and lean on the existing tests as the
regression guard.
- If `env_parsing.rs` helpers become unused after the migration, remove them in
the same change; if still used elsewhere, leave them.
- Mirrors the same cleanup already applied to `ConnectionPoolOptions` /
`DiagnosticsOptions` / runtime refresh interval in PR #4562 — follow that
precedent for structure and naming.
## Links
- Refactor origin PR:
- Related bug:
- Related bug fix (land first):
- File: `sdk/cosmos/azure_data_cosmos_driver/src/options/partition_failover.rs`
- Macro: `sdk/cosmos/azure_data_cosmos_macros` (`#[derive(CosmosOptions)]`)
- Macro tests showing `env_only` / `parser` / `overridable`:
`sdk/cosmos/azure_data_cosmos_macros/tests/derive_cosmos_options.rs`
Contributor guide
Assessment
This issue has not been assessed yet.