Skip to content

feat(config): parse MultiSubnetFailover instead of dropping it - #2

Open
claudespice wants to merge 1 commit into
spiceai:spiceaifrom
claudespice:fix/multi-subnet-failover
Open

feat(config): parse MultiSubnetFailover instead of dropping it#2
claudespice wants to merge 1 commit into
spiceai:spiceaifrom
claudespice:fix/multi-subnet-failover

Conversation

@claudespice

Copy link
Copy Markdown

Summary

MultiSubnetFailover parses into the connection-string dictionary and is then dropped. Config exposes no way to read it, so a caller cannot honour a setting the user explicitly asked for.

An Always-On availability group listener commonly resolves to one address per subnet — one in front of the primary replica, the rest passive or unreachable. A client that attempts them sequentially commits to each until the OS gives up, so a caller with its own connect deadline can time out while the live replica was never tried. MultiSubnetFailover=Yes is the ADO.NET keyword that asks for those attempts to be made concurrently, which is why users set it.

Changes

  • Config carries multi_subnet_failover, with Config::multi_subnet_failover(bool) to set it and Config::get_multi_subnet_failover() to read it — matching the existing readonly setter and get_addr getter conventions.
  • ConfigString::multi_subnet_failover() reads the multisubnetfailover key. Both parsers lower-case their keys, so the same lookup serves ADO.NET's MultiSubnetFailover and JDBC's multiSubnetFailover.
  • from_config_string populates it, so it survives both from_ado_string and from_jdbc_string.
  • Documented in the from_ado_string parameter table.

A non-boolean value is a Conversion error via the existing parse_bool, rather than a silent false — the caller asked for something specific and should hear that it was not understood.

Establishing the connection remains the caller's job. This change only makes the requested setting readable; it does not alter how or when tiberius dials, so behaviour is unchanged for every existing connection string. The default is false, which is today's sequential behaviour.

Why

spiceai/spiceai's MSSQL connector dials with TcpStream::connect(config.get_addr()), which resolves and attempts addresses sequentially. With a multi-subnet listener behind a 30s pool timeout, a passive address sorting first consumes the whole budget. The connector cannot act on MultiSubnetFailover because the value is not reachable from Config; parsing the connection string a second time on that side would split the source of truth for connection settings across two parsers. See spiceai/spiceai#12605.

Test plan

cargo test --no-default-features --features tds73,rustls,chrono --lib — 128 passed, 0 failed, including 11 new tests:

  • ado_net: Yes/true/No/false values, case-insensitive key, absent key defaults off, non-boolean rejected.
  • jdbc: multiSubnetFailover=true/false, absent key defaults off.
  • config: the setting survives from_ado_string and from_jdbc_string, defaults off and is not implied by an unrelated string, can be set directly, and a non-boolean fails the whole connection string.

cargo fmt --check clean. cargo clippy --lib --tests reports nothing in the three touched files (the pre-existing warnings elsewhere are untouched).

Based on the spiceai branch, at the revision spiceai/spiceai currently pins (9ae93c6).

An Always-On availability group listener commonly resolves to one address
per subnet, only one of which accepts connections. A client that tries
them sequentially stalls on each unreachable address until the OS gives
up, so a caller with its own connect deadline can time out before the
live replica is ever tried. MultiSubnetFailover is the ADO.NET keyword
that asks for those attempts to be made concurrently.

The key parsed into the connection-string dictionary and was then
dropped: Config exposed no way to read it, so a caller could not honour
it even though the user had asked for it.

Carry it on Config with a setter and a getter, populated from both the
ADO.NET and JDBC strings (one lookup serves both, since each parser
lower-cases its keys). A non-boolean value is a conversion error rather
than a silent false.

Opening the connection stays the caller's job -- this only makes the
requested setting readable.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Exposes MultiSubnetFailover through Config, enabling downstream connectors to implement concurrent address dialing for multi-subnet SQL Server listeners.

Changes:

  • Parses and stores the ADO.NET/JDBC setting with a default of false.
  • Adds public setter/getter methods and documentation.
  • Adds coverage for valid, missing, invalid, and case-insensitive values.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
src/client/config.rs Adds configuration storage, API access, parsing, documentation, and integration tests.
src/client/config/ado_net.rs Tests ADO.NET parsing behavior and validation.
src/client/config/jdbc.rs Tests JDBC parsing and default behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@claudespice

Copy link
Copy Markdown
Author

Waiting on a review that has never been requested

Recording the state so this stops sitting silently. Six days open, MERGEABLE/BLOCKED, and the block is entirely REVIEW_REQUIRED:

  • No checks run on this repo at allstatusCheckRollup is empty, so there is no red to chase and nothing to re-run.
  • No reviewer has ever been requested, by anyone. This is not a reviewer who has gone quiet; nobody was ever asked.
  • No assignee, and I cannot set one — the same account permission that denies AddLabelsToLabelable on spiceai/vortex applies here, so the ownership self-heal that would normally make this PR findable on later sweeps cannot run.

Nothing else is blocking it. The change parses MultiSubnetFailover from the connection string instead of silently dropping it; the diff is confined to config parsing.

A maintainer requesting a review — or granting the bot account triage rights on this repo, which would let it keep its own PRs assigned and labelled here as it does elsewhere — clears it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants