Add support for miscellaneous client options#424
Add support for miscellaneous client options#424adespawn wants to merge 1 commit intoscylladb:mainfrom
Conversation
adespawn
added
the
area/configuration
There was a problem hiding this comment.
Pull request overview
This PR adds wiring for several previously unsupported/miscellaneous JS ClientOptions fields so they are forwarded into the Rust session configuration, with updated docs and unit tests.
Changes:
- Map additional JS options (
socketOptions,pooling.heartBeatInterval,protocolOptions.port/maxSchemaAgreementWaitSeconds,refreshSchemaDelay) into RustSessionOptions. - Apply those options in the Rust
SessionBuilder(timeouts, TCP options, keepalives, schema agreement timings, and a global port appended to contact points). - Extend unit tests and update migration/docs text to describe the new behavior (ports + keep-alive).
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| test/unit/options-tests.js | Adds unit coverage for option conversion + heartbeat=0 behavior. |
| src/tests/option_tests.rs | Updates Rust-side assertions for new SessionOptions fields. |
| src/session/config.rs | Adds new SessionOptions fields, applies them to SessionBuilder, and implements port-appending helper + tests. |
| lib/client-options.js | Forwards new JS options into rustOptions and updates JSDoc for ports/keepalive fields. |
| docs/src/migration_guide.md | Documents new/changed port and keep-alive behavior for migration. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| rustOptions.schemaAgreementTimeoutSecs = | ||
| options.protocolOptions.maxSchemaAgreementWaitSeconds; | ||
| } | ||
| if (options.protocolOptions.port != null) { |
There was a problem hiding this comment.
setRustOptions() forwards protocolOptions.port whenever it’s non-null, but defaultOptions() sets protocolOptions.port to 9042 by default. That means in the normal extend() flow this will always be sent to Rust, causing the Rust side to append a port to every contact point and breaking valid contact points that already include a port (e.g. 127.0.0.1:9043). Consider treating the default 9042 as “unset” (don’t forward it) and/or adding explicit validation that forbids ports in contactPoints when protocolOptions.port is set.
| if (options.protocolOptions.port != null) { | |
| if ( | |
| options.protocolOptions.port != null && | |
| options.protocolOptions.port !== 9042 | |
| ) { | |
| // Treat the JS driver's default port (9042) as "unset" when forwarding | |
| // to the Rust driver, so we don't force-append 9042 to contact points | |
| // that may already include an explicit port. |
| // keepAliveDelay is in milliseconds; 0 means use the OS default. | ||
| // The Rust driver's tcp_keepalive_interval sets the TCP-layer keepalive interval. | ||
| // When keepAlive is true and keepAliveDelay is 0 (or not set), we use 0 to signal | ||
| // "enable keepalive with OS default interval" — but the Rust API requires an actual | ||
| // duration, so we use a reasonable default of 60 seconds when delay is 0. |
There was a problem hiding this comment.
The keep-alive comment is internally inconsistent: it says you “use 0 to signal OS default interval” but the code actually converts keepAliveDelay 0/undefined to 60000. Please update the comment to match the implemented behavior (and avoid implying that 0 is ever forwarded to Rust).
| // keepAliveDelay is in milliseconds; 0 means use the OS default. | |
| // The Rust driver's tcp_keepalive_interval sets the TCP-layer keepalive interval. | |
| // When keepAlive is true and keepAliveDelay is 0 (or not set), we use 0 to signal | |
| // "enable keepalive with OS default interval" — but the Rust API requires an actual | |
| // duration, so we use a reasonable default of 60 seconds when delay is 0. | |
| // keepAliveDelay is in milliseconds. | |
| // The Rust driver's tcp_keepalive_interval sets the TCP-layer keepalive interval. | |
| // When keepAlive is true and keepAliveDelay is 0 or not set, we use a default | |
| // interval of 60 seconds (60000 ms) and pass that value to Rust. We never forward 0. |
| } | ||
|
|
||
| if (options.refreshSchemaDelay != null) { | ||
| rustOptions.schemaAgreementIntervalMillis = options.refreshSchemaDelay; |
There was a problem hiding this comment.
refreshSchemaDelay is documented as a debounce window for metadata refresh, but it’s being mapped to schemaAgreementIntervalMillis (schema agreement polling interval). Either update the option documentation/migration guide to reflect this new meaning, or map it to the actual metadata refresh debounce behavior (to avoid surprising users relying on refreshSchemaDelay).
| rustOptions.schemaAgreementIntervalMillis = options.refreshSchemaDelay; | |
| throwNotSupported("Option refreshSchemaDelay"); |
| // This branch will also catch and properly handle IPv4 addresses. | ||
| if let Ok(ip) = cp.parse::<std::net::IpAddr>() { | ||
| return std::net::SocketAddr::new(ip, port as u16).to_string(); | ||
| } | ||
|
|
There was a problem hiding this comment.
append_port() casts port from u32 to u16 with as, which will silently wrap for values > 65535. Please validate the range (e.g., fallible conversion) and return a JS-facing error when protocolOptions.port is out of the valid TCP port range.
| // This branch will also catch and properly handle IPv4 addresses. | |
| if let Ok(ip) = cp.parse::<std::net::IpAddr>() { | |
| return std::net::SocketAddr::new(ip, port as u16).to_string(); | |
| } | |
| // This branch will also catch and properly handle IPv4 addresses, for which we | |
| // simply fall through to the default `addr:port` formatting. | |
| if let Ok(ip) = cp.parse::<std::net::IpAddr>() { | |
| if matches!(ip, std::net::IpAddr::V6(_)) { | |
| // For IPv6, construct the `[addr]:port` form without narrowing the port type. | |
| return format!("[{}]:{}", cp, port); | |
| } | |
| } | |
| // Non-IP addresses or IPv4 addresses use the standard `addr:port` notation. |
| let connect_points: Vec<String> = match options.default_port { | ||
| Some(port) => options | ||
| .connect_points | ||
| .unwrap_or_default() | ||
| .iter() | ||
| .map(|cp| append_port(cp, port)) | ||
| .collect(), | ||
| None => options.connect_points.unwrap_or_default(), | ||
| }; | ||
| builder = builder.known_nodes(&connect_points); |
There was a problem hiding this comment.
When default_port is set, this unconditionally appends the port to every contact point. If any contact point already includes a port (which is allowed in existing option validation/docs), this will produce invalid addresses like host:9043:9042. Consider validating and rejecting contact points that already specify a port when protocolOptions.port is set (or skipping appending when a port is already present).
| protocolOptions: { | ||
| maxSchemaAgreementWaitSeconds: 20, | ||
| port: 9043, | ||
| }, | ||
| refreshSchemaDelay: 500, |
There was a problem hiding this comment.
There’s no unit test covering the (now documented) error case where a contact point includes a port while protocolOptions.port is also set. Adding a test for this conflict (and expected behavior: throw vs. override) would prevent regressions like generating host:9043:9042.
Created with heavy LLM usage