Driver config reporting — stage 2: full DRIVER_CONFIG report - #263
Open
sylwiaszunejko wants to merge 7 commits into
Open
Driver config reporting — stage 2: full DRIVER_CONFIG report#263sylwiaszunejko wants to merge 7 commits into
sylwiaszunejko wants to merge 7 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe driver now builds v1 Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
Comment |
There was a problem hiding this comment.
Pull request overview
Implements the full schema-based DRIVER_CONFIG payload for control connections.
Changes:
- Serializes effective connection, control-plane, query, TLS, and policy settings.
- Tracks TLS hostname verification and explicit datacenter configuration.
- Adds schema-conformance, unit, and integration coverage.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/Cassandra/SSLOptions.cs |
Exposes hostname-verification state. |
src/Cassandra/Requests/DriverConfigReporter.cs |
Builds the full configuration report. |
src/Cassandra/Policies/DCAwareRoundRobinPolicy.cs |
Tracks explicit local-DC configuration. |
src/Cassandra/Configuration.cs |
Supplies configuration to the reporter. |
src/Cassandra.Tests/Requests/StartupOptionsFactoryTests.cs |
Updates startup-option tests. |
src/Cassandra.Tests/Requests/DriverConfigReporterTests.cs |
Adds comprehensive reporter tests. |
src/Cassandra.Tests/Requests/driver-config-report-v1.schema.json |
Adds the normative v1 schema. |
src/Cassandra.Tests/Cassandra.Tests.csproj |
Embeds the schema and adds its validator. |
src/Cassandra.IntegrationTests/Core/StartupOptionsTests.cs |
Verifies reports over real connections. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
src/Cassandra.IntegrationTests/Core/StartupOptionsTests.cs-141-151 (1)
141-151: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winRetain the startup record when validating
DRIVER_CONFIG.The projection discards
RequestLog.Connection, so the helper cannot identify the sender. Correlate the sole report with the control connection and assert that pool connections do not reportDRIVER_CONFIG.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Cassandra.IntegrationTests/Core/StartupOptionsTests.cs` around lines 141 - 151, The GetDriverConfigReportAsync helper must retain each startup log’s RequestLog.Connection while extracting DRIVER_CONFIG. Correlate the single report with the control connection, assert pool connections do not contain DriverConfigReporter.DriverConfigOption, then parse and return the control-connection report.
🧹 Nitpick comments (1)
src/Cassandra.Tests/Requests/DriverConfigReporterTests.cs (1)
363-375: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the schema conformance assertion to this test.
Nearly every sibling test ends with
AssertConformsToSchema(report). This test omits it. Buffer omission is exactly the path where a required sub-object field could go missing and break the schema, so the assertion has value here. The same assertion is also missing inShould_ReportTheProfileReadTimeout_When_TheDefaultProfileOverridesIt,Should_ReportACustomRetryPolicy_By_TheDecoratorName_When_ItDecoratesACustomPolicy,Should_ReportACustomLoadBalancingPolicy_By_TheOutermostConfiguredName, andShould_ReportHostnameVerification_When_TheValidationCallbackIsNull.♻️ Proposed change
Assert.IsNull(report["connection"]["socket"]["receive-buffer"]); Assert.IsNull(report["connection"]["socket"]["send-buffer"]); + DriverConfigReporterTests.AssertConformsToSchema(report); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Cassandra.Tests/Requests/DriverConfigReporterTests.cs` around lines 363 - 375, Add AssertConformsToSchema(report) to Should_OmitTheBufferSizes_When_TheyAreNotPositive and the four named sibling tests: Should_ReportTheProfileReadTimeout_When_TheDefaultProfileOverridesIt, Should_ReportACustomRetryPolicy_By_TheDecoratorName_When_ItDecoratesACustomPolicy, Should_ReportACustomLoadBalancingPolicy_By_TheOutermostConfiguredName, and Should_ReportHostnameVerification_When_TheValidationCallbackIsNull, placing each assertion after the existing report assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Other comments:
In `@src/Cassandra.IntegrationTests/Core/StartupOptionsTests.cs`:
- Around line 141-151: The GetDriverConfigReportAsync helper must retain each
startup log’s RequestLog.Connection while extracting DRIVER_CONFIG. Correlate
the single report with the control connection, assert pool connections do not
contain DriverConfigReporter.DriverConfigOption, then parse and return the
control-connection report.
---
Nitpick comments:
In `@src/Cassandra.Tests/Requests/DriverConfigReporterTests.cs`:
- Around line 363-375: Add AssertConformsToSchema(report) to
Should_OmitTheBufferSizes_When_TheyAreNotPositive and the four named sibling
tests: Should_ReportTheProfileReadTimeout_When_TheDefaultProfileOverridesIt,
Should_ReportACustomRetryPolicy_By_TheDecoratorName_When_ItDecoratesACustomPolicy,
Should_ReportACustomLoadBalancingPolicy_By_TheOutermostConfiguredName, and
Should_ReportHostnameVerification_When_TheValidationCallbackIsNull, placing each
assertion after the existing report assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: QUIET
Plan: Pro Plus
Run ID: 9f8c585e-23c3-4906-aaa3-a6634ea9f038
📒 Files selected for processing (9)
src/Cassandra.IntegrationTests/Core/StartupOptionsTests.cssrc/Cassandra.Tests/Cassandra.Tests.csprojsrc/Cassandra.Tests/Requests/DriverConfigReporterTests.cssrc/Cassandra.Tests/Requests/StartupOptionsFactoryTests.cssrc/Cassandra.Tests/Requests/driver-config-report-v1.schema.jsonsrc/Cassandra/Configuration.cssrc/Cassandra/Policies/DCAwareRoundRobinPolicy.cssrc/Cassandra/Requests/DriverConfigReporter.cssrc/Cassandra/SSLOptions.cs
sylwiaszunejko
force-pushed
the
driver-config-stage-2
branch
from
August 5, 2026 07:51
cf97634 to
7bd194f
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Cassandra.IntegrationTests/Core/StartupOptionsTests.cs`:
- Around line 141-173: Update
Should_ReportDriverConfig_OnlyOnTheControlConnection to call
GetDriverConfigReportAsync for retrieving the DRIVER_CONFIG report, removing its
duplicated startup-log parsing and single-report assertions. Keep only the
existing version assertion in the test, while relying on the helper’s stricter
connection validation.
In `@src/Cassandra/Requests/DriverConfigReporter.cs`:
- Around line 326-341: Make DriverConfigReporter tolerate absent policies so one
missing policy does not discard the complete report: in
src/Cassandra/Requests/DriverConfigReporter.cs lines 326-341, update
Query/LoadBalancingPolicy handling to return an absent or custom group for an
empty PolicyChain instead of indexing it; in lines 408-433, skip the
reconnection group when Policies.ReconnectionPolicy is null; and in lines
435-476, return early from the retry-policy handling when its chain is empty
before accessing chain[0].
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: QUIET
Plan: Pro Plus
Run ID: 9b0a77a3-a575-4722-bfa6-29290786ce66
📒 Files selected for processing (9)
src/Cassandra.IntegrationTests/Core/StartupOptionsTests.cssrc/Cassandra.Tests/Cassandra.Tests.csprojsrc/Cassandra.Tests/Requests/DriverConfigReporterTests.cssrc/Cassandra.Tests/Requests/StartupOptionsFactoryTests.cssrc/Cassandra.Tests/Requests/driver-config-report-v1.schema.jsonsrc/Cassandra/Configuration.cssrc/Cassandra/Policies/DCAwareRoundRobinPolicy.cssrc/Cassandra/Requests/DriverConfigReporter.cssrc/Cassandra/SSLOptions.cs
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Suppressed comments (5)
src/Cassandra/Requests/DriverConfigReporter.cs:259
- This reports configured intent rather than effective state.
ReuseAddressis never read outsideSocketOptionsand this reporter, soSetReuseAddress(true)does not enableSO_REUSEADDR, yet the report says it is enabled; that contradicts both the PR's effective-configuration contract and the schema. Either apply the option inTcpSocketor report the actual default state.
// ReuseAddress is the exception: the driver accepts it but never applies SO_REUSEADDR, so the
// effective state is always the platform default. What is reported is the configured intent, both
// because that is what the sibling drivers report and because it is the only thing here that carries
// any information; the platform default would be a constant false.
socket["reuse-address"] = options.ReuseAddress ?? false;
src/Cassandra/Requests/DriverConfigReporter.cs:173
- A zero connect timeout is not disabled:
TcpSocket.Connectpasses it toTaskCompletionSourceWithTimeout, whoseTimer.Change(0, ...)schedules an immediate timeout (src/Cassandra/Tasks/TaskHelper.cs:411). This branch therefore omits a real zero-duration bound. Report zero after allowing it in the schema, or reject non-positive connect timeouts when they are configured.
if (socketOptions.ConnectTimeoutMillis > 0)
{
connect["timeout-ms"] = socketOptions.ConnectTimeoutMillis;
src/Cassandra/Requests/DriverConfigReporter.cs:371
int.MaxValuemeans paging is disabled (QueryOptions.cs:130), while the schema says thepagegroup is absent when paging is not limited. This condition emitspage.size = 2147483647instead, misrepresenting an unlimited default as a finite page size. Excludeint.MaxValueand update the test that currently expects it to be reported.
// QueryOptions rejects a non-positive page size and spells "do not page" as int.MaxValue, which the
// schema admits, so this guard only keeps the optional group from carrying a value the schema would
// reject if a page size ever reached here from somewhere that does not validate it.
if (requestOptions.PageSize > 0)
{
queryDefaults["page"] = new JObject { ["size"] = requestOptions.PageSize };
src/Cassandra/Requests/DriverConfigReporter.cs:392
- This is false for a supported custom timestamp generator that returns
long.MinValue: the driver then omits the protocol timestamp and lets the server assign it (QueryProtocolOptions.cs:125-129), and the test suite already has aNoTimestampGeneratordemonstrating this behavior (RequestHandlerTests.cs:304-310). Do not hard-codetrue; reconcile custom-generator behavior with the schema's required boolean.
// The driver assigns the write timestamp client-side through the timestamp generator whenever the
// protocol supports it. A generator can opt out per request by returning long.MinValue, but that is
// a runtime decision rather than a configured one and there is no built-in generator that does it,
// so client-side timestamps are what this configuration says.
queryDefaults["client-timestamps"] = true;
src/Cassandra/Requests/DriverConfigReporter.cs:502
IdempotenceAwareRetryPolicyis not a transparent decorator: it forcesRethrowfor non-idempotent write timeouts and request errors (IdempotenceAwareRetryPolicy.cs:53-79). Walking through it can therefore report its child asstandard-error-aware(or another built-in) even though the effective retry behavior differs. Stop the chain at this policy and report it as custom unless the schema gains an idempotence-aware shape.
else if (current is IdempotenceAwareRetryPolicy idempotenceAware)
{
current = idempotenceAware.ChildPolicy;
}
nikagra
reviewed
Aug 5, 2026
nikagra
left a comment
nikagra
left a comment
There was a problem hiding this comment.
Reviewed the full diff and cross-checked the reconciliation claims against the driver source; findings are inline.
| } | ||
|
|
||
| #pragma warning disable 618 | ||
| var dcFailover = dcAware != null && dcAware.UsedHostsPerRemoteDc > 0; |
There was a problem hiding this comment.
dc-failover is misreported when the chain has no DC-aware child. TokenAwarePolicy(new RoundRobinPolicy()) reports dc-failover: false, but RoundRobinPolicy.Distance returns Local for every host in every datacenter, so every query plan spans all DCs.
The same rule also launders a custom child into the built-in shape: TokenAwarePolicy(someCustomPolicy) reports type: "token-aware" and the custom policy's name never appears in the report. The retry path deliberately avoids exactly this by naming the outermost policy the application configured.
Suggestion: claim token-aware only when the chain terminates in a policy the reporter recognizes (DCAwareRoundRobinPolicy), otherwise fall through to CustomPolicy(chain[0]), and derive dc-failover from the chain rather than from the presence of a DC-aware policy. No test currently covers TokenAwarePolicy over a non-DC-aware child.
Author
There was a problem hiding this comment.
regarding dc-failover we agreed here #263 (comment) to have it false for RoundRobin
| @@ -0,0 +1,983 @@ | |||
| { | |||
| "$schema": "https://json-schema.org/draft/2020-12/schema", | |||
| "$id": "https://scylladb.com/schemas/driver-client-options/v1.json", | |||
There was a problem hiding this comment.
This copy is a later revision than the one java-driver#968 vendors, under the same version: 1. Diffing the two:
| java-driver#968 | here | |
|---|---|---|
| top level | 9 required groups, flat | 4; socket/retry-policy/load-balancing-policy/node-location-preference/query-defaults/tls moved under connection.* / query.* |
control-plane |
system-queries, schema-agreement |
queries.system, schema.agreement |
tls |
required enabled + hostname-verification |
no enabled; group omitted when off |
retry-policy |
5 variants, per-policy backoff |
6 variants (adds never); backoff hoisted to query.retry.backoff |
adaptive-ordering |
required enabled |
required signals, no enabled |
constant retry backoff.delay-ms |
nonNegativeInteger |
positiveInteger |
Not a defect in this diff — this branch is ~9h newer, so it looks like a revision the java leg hasn't picked up yet. But as it stands the two legs would put structurally different documents on the wire under the same version, and whichever lands first is what a server-side consumer has to parse. Which revision is normative, and are java-driver#968 / gocql stage 2 following it?
dkropachev
reviewed
Aug 5, 2026
| // protocol supports it. A generator can opt out per request by returning long.MinValue, but that is | ||
| // a runtime decision rather than a configured one and there is no built-in generator that does it, | ||
| // so client-side timestamps are what this configuration says. | ||
| queryDefaults["client-timestamps"] = true; |
Collaborator
There was a problem hiding this comment.
Also false on protocol v1/v2: SupportsTimestamp() starts at v3, so the generator never contributes a protocol timestamp. Derive this from negotiated protocol as well as generator.
Author
There was a problem hiding this comment.
AFAIK we don't support protocol v1/v2
| // to whatever it wraps. | ||
| foreach (var policy in chain) | ||
| { | ||
| if (policy is DefaultRetryPolicy) |
Collaborator
There was a problem hiding this comment.
DefaultRetryPolicy does not implement the schema's standard-error-aware rules: it never retries Unavailable and retries request errors regardless of idempotence. Report it as custom or add a matching schema type.
Author
There was a problem hiding this comment.
The current standard-error-aware description is, in full: "Standard error-aware retry policy." So DefaultRetryPolicy — the driver's standard retry policy, dispatching per error type — fits the description that's actually there. Reporting custom here would make the standard-error-aware branch dead code in this driver, so every default cluster — the overwhelming majority — would report custom. That destroys the most useful signal in the retry group: whether the client runs the driver default or a bespoke policy.
dkropachev
reviewed
Aug 5, 2026
sylwiaszunejko
force-pushed
the
driver-config-stage-2
branch
from
August 6, 2026 10:32
7bd194f to
7de3d67
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
Suppressed comments (3)
src/Cassandra/Requests/DriverConfigReporter.cs:427
client-timestampsis not always enabled. A cluster forced to protocol V1/V2 viaWithMaxProtocolVersionalways uses server-side timestamps (Builder.cs:632-634), andQueryProtocolOptions.cs:123-129only invokes the timestamp generator when the negotiated protocol supports timestamps. This report therefore misstates that configuration; derive the value from the protocol used for this STARTUP request instead of emitting a constant.
queryDefaults["client-timestamps"] = true;
src/Cassandra/Requests/DriverConfigReporter.cs:715
- Reaching this limit leaves
chainlooking complete. For example, 16 nestedTokenAwarePolicywrappers around a custom policy stop before the custom child, soLoadBalancingPolicysees only recognized policies and incorrectly reports the built-in token-aware shape. Do not classify a truncated chain; fail report construction or propagate a truncation flag so it is reported conservatively.
if (current != null)
{
DriverConfigReporter.Logger.Warning(
"Stopped walking the load balancing policy chain after {0} policies, only those are reported.",
DriverConfigReporter.MaxPolicyChainLength);
src/Cassandra/Requests/DriverConfigReporter.cs:235
- This configured pooling threshold is not necessarily the effective per-connection in-flight capacity. For protocol V1/V2,
Connection.GetMaxConcurrentRequestsallocates only 128 stream IDs (Connection.cs:221-230), while the default value emitted here is 2048. Report the minimum of the configured threshold and the capacity for the protocol used by this connection.
var maxRequests = pooling.GetMaxRequestsPerConnection();
sylwiaszunejko
force-pushed
the
driver-config-stage-2
branch
from
August 6, 2026 11:48
7de3d67 to
e3a5367
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.
Suppressed comments (4)
src/Cassandra/Builder.cs:736
- This accepts every positive
int, but accepted values aboveint.MaxValue / 2still break synchronous schema refresh:GetQueryAbortTimeout(2)multiplies unchecked, soWithQueryTimeout(int.MaxValue)produces-2andTask.Waitthrows. Make timeout scaling overflow-safe (and cover the boundary), or reject values that cannot be scaled by existing callers.
if (queryAbortTimeout != Timeout.Infinite && queryAbortTimeout <= 0)
src/Cassandra/Requests/DriverConfigReporter.cs:58
- The reporter still transmits documents that it knows the shipped normative schema rejects (for example,
in-flight.maxof 0/40000 or a serial default consistency). A consumer validating v1 must reject the entireDRIVER_CONFIG, so this does not reliably satisfy the PR's schema-shaped report contract. Please either validate these public configuration inputs before cluster creation or establish a schema-valid representation before sending the report.
/// Such a value is reported <em>as-is</em>: fabricating an in-range one would misreport a setting the
/// operator may have chosen deliberately, and dropping the whole report over a single field would lose
/// everything else. The document is then accurate but fails validation on that one field, which is tracked
/// as a cross-driver schema gap.
src/Cassandra/Requests/DriverConfigReporter.cs:698
- An empty datacenter is not auto-discovered:
DCAwareRoundRobinPolicy.Initializeonly infers when_localDc == nulland otherwise rejects an unmatched empty value. Reportingdc-autohere therefore claims behavior that will not occur. Distinguish null from empty; omitting the optional preference for the unrepresentable empty value avoids the false claim.
var localDc = dcAware.LocalDc;
if (string.IsNullOrEmpty(localDc))
{
return new JObject { ["type"] = "dc-auto" };
src/Cassandra/Requests/DriverConfigReporter.cs:728
- Returning the truncated list treats an incomplete chain as fully inspected. For example, 16 public
TokenAwarePolicywrappers around a custom or DC-aware tail are all marked recognized, so the report claims the built-in token-aware shape and can omit the effective DC preference even though the unvisited tail controls it. Propagate that traversal was truncated and report the chain as custom (or suppress the report) instead of deriving capabilities from partial data.
if (current != null)
{
DriverConfigReporter.Logger.Warning(
"Stopped walking the load balancing policy chain after {0} policies, only those are reported.",
DriverConfigReporter.MaxPolicyChainLength);
dkropachev
reviewed
Aug 6, 2026
sylwiaszunejko
force-pushed
the
driver-config-stage-2
branch
from
August 6, 2026 17:07
e3a5367 to
d9a7849
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
src/Cassandra/Requests/DriverConfigReporter.cs-833-835 (1)
833-835: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winLog the unrepresentable consistency level. The fallback emits a number, which the schema
consistencyenum rejects. Every other schema-invalid value callsWarnUnrepresentable. Add the same call here so the mismatch is visible in the log.🩹 Proposed fix
default: // Not a level the driver defines; report the number so the report stays truthful. + DriverConfigReporter.WarnUnrepresentable( + "query.defaults.consistency", (int)consistency, "the schema lists only the named levels"); return ((int)consistency).ToString();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Cassandra/Requests/DriverConfigReporter.cs` around lines 833 - 835, Update the fallback branch in the consistency reporting method to call WarnUnrepresentable before returning the numeric consistency value. Preserve the existing numeric return so the report remains truthful, while ensuring the schema-invalid level is logged consistently with the other invalid values.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Cassandra/Builder.cs`:
- Around line 718-743: Update the metadata wait logic in Metadata.GetTable and
Metadata.GetMaterializedView to obtain the timeout via
Configuration.DefaultRequestOptions.GetQueryAbortTimeout(2) instead of
multiplying _queryAbortTimeout directly, preserving Timeout.Infinite as a valid
indefinite wait.
---
Other comments:
In `@src/Cassandra/Requests/DriverConfigReporter.cs`:
- Around line 833-835: Update the fallback branch in the consistency reporting
method to call WarnUnrepresentable before returning the numeric consistency
value. Preserve the existing numeric return so the report remains truthful,
while ensuring the schema-invalid level is logged consistently with the other
invalid values.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: QUIET
Plan: Pro Plus
Run ID: 32eb9203-fc56-4642-8ffe-a53359b2b4b1
📒 Files selected for processing (16)
src/Cassandra.IntegrationTests/Core/StartupOptionsTests.cssrc/Cassandra.Tests/BuilderTests.cssrc/Cassandra.Tests/Cassandra.Tests.csprojsrc/Cassandra.Tests/ExecutionProfiles/RequestOptionsTests.cssrc/Cassandra.Tests/Requests/DriverConfigReporterTests.cssrc/Cassandra.Tests/Requests/StartupOptionsFactoryTests.cssrc/Cassandra.Tests/Requests/driver-config-report-v1.schema.jsonsrc/Cassandra.Tests/SocketOptionsTests.cssrc/Cassandra/Builder.cssrc/Cassandra/Configuration.cssrc/Cassandra/Connections/Connection.cssrc/Cassandra/Metadata.cssrc/Cassandra/Policies/DCAwareRoundRobinPolicy.cssrc/Cassandra/Requests/DriverConfigReporter.cssrc/Cassandra/SSLOptions.cssrc/Cassandra/SocketOptions.cs
A query timeout of 0 was accepted and then honoured literally: the synchronous paths hand it to Task.Wait, which returns immediately, so every request failed with a TimeoutException before it could complete. Anything below Timeout.Infinite is worse still, making Task.Wait throw. Neither is a configuration anyone can have meant, and both only showed up at the first query rather than where the mistake was made. Only a positive number of milliseconds and Timeout.Infinite, which waits indefinitely, are now accepted. The check sits in Builder.WithQueryTimeout, following WithMaxSchemaAgreementWaitSeconds, and is repeated in Configuration's constructor because ClientOptions is public and can be handed straight to it, bypassing the builder: no cluster should come into existence in that state. Declaring Timeout.Infinite valid means honouring it everywhere, and three callers did not: Metadata.RefreshSchema, GetTable and GetMaterializedView each doubled the timeout by multiplying it, turning -1 into -2, which Task.Wait rejects outright — so a cluster configured to wait indefinitely could neither refresh its schema nor read table or view metadata synchronously. They now scale through RequestOptions.GetQueryAbortTimeout, which preserves the sentinel and which the equivalent two-query waits in KeyspaceMetadata already used. That helper had no tests despite now carrying the invariant, so it gets a fixture too. The other scaled timeouts are safe: HostConnectionPool clamps a non-positive drain delay to its maximum, and Cluster takes the greater of its scaled connect timeout and the metadata abort timeout. Note this rejects a call that used to be accepted, so an application passing 0 today, and failing every synchronous request because of it, now fails while the cluster is being configured instead.
Same hole as the query timeout, in the other timeout an application can set. SetConnectTimeoutMillis accepted 0 and the driver then honoured it literally: the value is handed to Timer.Change by TaskHelper.TaskCompletionSourceWithTimeout, so the timeout fired at once and every connection attempt failed with a SocketException before it could be established. TcpSocket.Connect, the SSL handshake and the startup and authentication requests in Connection all use it, so nothing could connect. Anything below Timeout.Infinite is worse still, making Timer.Change throw. Only a positive number of milliseconds and Timeout.Infinite, which waits indefinitely, are now accepted. One check is enough here, unlike the query timeout: SetConnectTimeoutMillis is the only writer of the field and SocketOptions has no constructor taking it, so no path can bypass it. SocketOptions had no tests at all, so this adds a fixture for the setting it now validates. Note this rejects a call that used to be accepted, though only one that could never have worked: a cluster configured this way established no connections whatsoever, and now says so while it is being configured.
The driver hands the server name to SslStream, which reports a name mismatch as SslPolicyErrors.RemoteCertificateNameMismatch. Both the callback SSLOptions installs by default and .NET's own validation, used when the callback is null, reject that, so the host name is verified in both cases; an application supplied callback decides for itself and what it decides is not introspectable. Answering that question needs the default callback to be recognizable, so it moves into a static field rather than being attached as a method group at every field initialization. Diagnostic only: nothing about how a connection is authenticated changes. The answer is therefore nullable: true for the driver's default callback and for null, and null for an application supplied one. Not false, because a callback that ignores a name mismatch cannot be told apart from one that enforces it, so the driver knows neither that the host name is checked nor that it goes unchecked. Needed by the driver configuration report, whose tls group reports host name verification only when it is known and omits the key otherwise.
LocalDc alone cannot answer this: Initialize overwrites the field with the datacenter of the host the control connection uses when none was configured, so an explicit preference and an inferred one look the same afterwards. Recorded in the constructor, where the distinction is still available, and kept internal since only the driver needs it. Needed by the driver configuration report, whose node-location-preference group distinguishes an explicit datacenter (dc) from an inferred one (dc-auto).
Embed the v1 DRIVER_CONFIG JSON Schema in the test assembly verbatim, so that the report can be checked against the normative document that the ScyllaDB drivers share rather than against a restatement of it in test code, and reference JsonSchema.Net to evaluate it. Pinned to the 8.0.x line, the last one published under a plain MIT license: from 9.0.0 the NuGet binaries carry an Open Source Maintenance Fee EULA that asks users with revenue-generating use and at least US$10,000 annual gross revenue for a monthly fee. The source stays MIT either way, and the agreement covers only the pre-compiled binaries, which is exactly what a PackageReference consumes. Referenced only for net8 and net9. It reaches System.Text.Json 10 and System.Collections.Immutable 10 through Json.More.Net, whose netstandard2.0 group requires them outright so they cannot be pinned lower, and those warn on net6 and net7. The report is one code path with no per-framework behaviour, so validating it on the newer targets establishes its conformance for all of them. Test scope only, so nothing is added to what the driver ships. Vendored data and project plumbing, no logic yet.
Replace the schema-version-only placeholder with the report itself, so that an operator investigating an incident can see how the client is actually configured. The v1 schema nests everything under three groups: connection (its timeouts, request capacity, pool, socket, reconnection policy and tls), control-plane (the system-query and schema-agreement timeouts) and query (the per-request defaults, the retry, load balancing and speculative execution policies, and the datacenter preference). Built from the default execution profile rather than from Policies and QueryOptions directly, because a profile can override the read timeout and the policies, and what applies to a request that names no profile is what this report describes. The reporter therefore holds the Configuration and reads it when it builds a report, which is on every control connection handshake, so an inferred datacenter is reported once it becomes known. Conventions the schema imposes: kebab-case keys, nested objects, and omission rather than null of anything with no value. That extends to a configured value the schema cannot express while the key is optional, so a disabled read timeout, a disabled SO_LINGER and a non-positive socket buffer are left out instead of being reported as a number the schema rejects. Where only the key is optional and its group is not, the group stays and the number goes: that is how a disabled connect timeout and an infinite request timeout are reported. Reconciling the driver's options with the schema: - connection.read and the query policies come from the default profile; query.defaults.request.timeout-ms is the whole-request bound (QueryAbortTimeout) while connection.read bounds a single host, so the two describe different settings rather than the same one twice. - connection.requests.in-flight.max is whichever of two per connection limits binds first: the threshold PoolingOptions configures, above which HostConnectionPool rejects a borrow, and the size of the stream identifier pool, which Connection fixes at 2048, or 128 for single-byte stream ids, whatever the pool is configured with. Past that ceiling requests wait for an identifier rather than travelling, so reporting the configured value alone would overstate what a connection can do. Connection gains a static overload of GetMaxConcurrentRequests so both it and the report read the same constant; the report assumes the highest supported protocol version, as the pooling defaults already do, which is the one ScyllaDB negotiates. orphaned.max is SocketOptions.DefunctReadTimeoutThreshold: the driver counts the operations that timed out without a response, whose stream identifiers it cannot reuse, and HostConnectionPool.CheckHealth closes and replaces the connection once it reaches that count, which is what the schema describes. A negative threshold behaves like 0, so it is clamped. - control-plane.queries.system reports MetadataAbortTimeout, the bound ControlConnection actually applies to internal queries. There is no client-configurable server-side timeout, so server-side-ms is omitted. - The load balancing group is derived from the whole policy chain, walked once and shared with the datacenter preference. The schema has exactly one built-in shape, the token-aware policy, so a chain with token awareness reports load-distribution "shuffle" — TokenAwarePolicy starts the local replicas of a query plan at a pseudo-random index rather than rotating them deterministically. Its flags describe the whole chain, so they are only filled in when every policy in it is one the driver knows; a chain without token awareness, or one reaching an application supplied policy whose query plans this code cannot see, is reported as custom and named after the outermost policy the application configured. Its datacenter preference survives that, node-preference being a sibling of the policy rather than part of it. fallback-to-non-preferred-nodes is whether a request may go to a node outside the reported preference: for a datacenter-aware policy, whether it keeps hosts per remote datacenter. Round robin reports false, not because it stays local — it marks every host local, so a query can land on a remote one — but because it declares no preference to fall outside of, and none is reported for such a chain. Cross-driver decision. adaptive-ordering is omitted: the driver does not reorder candidates on runtime signals. So is max-retries on every retry policy: the built-ins have fixed rules rather than a configurable limit, which is what the schema reads its absence as. connection.node-preference is omitted too — it describes a datacenter or rack set on the session or cluster itself, and this driver has no such setting, the preference living only in the load balancing policy. - The retry chain looks through the decorators that pass the decision through unchanged: LoggingRetryPolicy, which logs its child's decision and returns it, and the internal WrappedExtendedRetryPolicy the driver puts around a plain IRetryPolicy; without the latter, a policy such as DowngradingConsistencyRetryPolicy would be reported as custom. A decorator that overrides the decision is not looked through, and leaves the group reported as custom: IdempotenceAwareRetryPolicy rethrows non-idempotent write timeouts and request errors instead of asking its child, so naming the child's type would promise retry rules that two of the four decision points never reach. RetryLoadBalancingPolicy is excluded from the load balancing flags for the same reason, its query plan re-enumerating the child's in an unbounded loop and sleeping between passes. query.retry.backoff is omitted throughout, no built-in policy delays a retry. - FixedReconnectionPolicy is reported as custom: one delay per attempt with the last repeating forever matches no built-in shape. - connection.pool carries only shard-aware, which reports configuration intent; at runtime the port must also be advertised and reachable. Pooling defaults come from the highest supported protocol version, because PoolingOptions is null until one is negotiated. - connection.tls is absent when TLS is off, and otherwise reports only whether the host name is verified. - connection.write is omitted. TcpSocket does assign the connect timeout to the socket's SendTimeout, but .NET only honours that for synchronous sends while the driver writes asynchronously, so reporting it would claim a bound that is not in force under a key the application never set. connection.heartbeat is reserved-empty in v1. - socket.reuse-address is a constant false, the platform default, because the driver sets SO_REUSEADDR on no socket. Deliberately not derived from SocketOptions.ReuseAddress: that option never meant SO_REUSEADDR, having been handed to Socket.Disconnect(reuseSocket) until that code was replaced, and nothing has read it since. Reporting it would claim the flag is set on the client sockets when it never is. - query.defaults.page is omitted when paging is unlimited. int.MaxValue is how the driver spells that, and QueryProtocolOptions turns it into -1 and leaves the page-size flag unset, so no limit reaches the server and there is no bound to report. - query.defaults.client-timestamps and tls.hostname-verification are reported only when the driver knows the answer, and omitted otherwise, which the schema reads as unknown. An ITimestampGenerator hands assignment back to the coordinator by returning long.MinValue and may decide that per request; a certificate validation callback that ignores a name mismatch cannot be told apart from one that enforces it. In both cases an application supplied implementation leaves the driver unable to claim either answer, so it claims neither. Neither field is ever false: no configuration makes server-side timestamps or disabled verification knowable. The sibling java drivers can report false for timestamps because they have a ServerSideTimestampGenerator to recognize; this driver has no such class, so its own generators are what can be recognized. - A datacenter configured as the empty string is reported as no datacenter at all: the schema requires a non-empty name and the policy would reject it when it initializes anyway. One field carries a value the schema cannot express, the driver not validating it on the way in: the in-flight maximum, which must be positive while SetMaxRequestsPerConnection takes any int. It is reported as-is, because fabricating an in-range value would misreport a setting an operator may have chosen deliberately and dropping the whole report over one field would lose everything else, and a test asserts that replacing that single field makes the document conform so the violation stays pinned to it. It is also logged, so a report which will not validate is visible in the driver's log rather than only in this class's documentation. A serial default consistency needs no such handling: the driver supports it, RequestHandler routing such a request as an LWT, and the schema's enum lists both levels. query.defaults.request needs no such compromise: Timeout.Infinite is the one value meaning there is no bound, and both the group and its key are optional, so that case drops the group. Every other value reaching the report is positive, a query timeout that would fail every request being rejected when the cluster is configured. connection.connect.timeout-ms follows the same rule for the same reason. The report is validated against the normative v1 schema, including a negative test proving that additionalProperties:false is enforced and so that the conformance assertions are not vacuous.
Verify over an actual connection to Simulacron that what the builder is given is what the server is told: the configured timeouts, the request capacity, each policy in its discriminated shape, the explicit datacenter preference and the query defaults, plus the absence of the tls group when TLS is off. A second test covers the default policy chain, which infers the datacenter from the node the control connection uses and so reports the preference as dc-auto with no name yet, since the report is built before that node is known. Every claim these tests make about DRIVER_CONFIG is a claim about which connections carry it, so the startup options are kept paired with the connection that sent them and the number of distinct connections is what gets checked. Counting startup messages instead, as an earlier draft did, would also be satisfied by a run where only the control connection had opened, or where one connection sent two of them. The SESSION_ID test that asserts every connection agrees is routed through the same helper, for the same reason. The shape of the whole document and its conformance to the schema are covered by the unit tests; these only pin that the report survives the wire intact.
sylwiaszunejko
force-pushed
the
driver-config-stage-2
branch
from
August 6, 2026 17:29
d9a7849 to
fc0042d
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/Cassandra/Requests/DriverConfigReporter.cs (1)
833-835: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLog the unrepresentable consistency value. The default branch emits a number that the schema
consistencyenum rejects. Every other schema-invalid value callsWarnUnrepresentable, so this path is silently inconsistent.QueryOptions.SetConsistencyLevelaccepts any cast enum value, so the branch is reachable.♻️ Proposed change
default: // Not a level the driver defines; report the number so the report stays truthful. + DriverConfigReporter.WarnUnrepresentable( + "query.defaults.consistency", (int)consistency, "the schema lists only the named levels"); return ((int)consistency).ToString();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Cassandra/Requests/DriverConfigReporter.cs` around lines 833 - 835, Update the default branch of the consistency formatting logic in DriverConfigReporter to call WarnUnrepresentable for undefined consistency values before returning the numeric representation. Preserve the truthful numeric output while matching the handling used by other schema-invalid values.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Cassandra/Metadata.cs`:
- Around line 355-358: Prevent overflow in GetQueryAbortTimeout when scaling a
valid finite timeout such as int.MaxValue: clamp scaled results to int.MaxValue
(or reject unscalable values) while preserving Timeout.Infinite. Ensure
GetTable, GetMaterializedView, and RefreshSchema pass only valid Task.Wait
timeouts, and add coverage for WithQueryTimeout(int.MaxValue) with scaling.
---
Nitpick comments:
In `@src/Cassandra/Requests/DriverConfigReporter.cs`:
- Around line 833-835: Update the default branch of the consistency formatting
logic in DriverConfigReporter to call WarnUnrepresentable for undefined
consistency values before returning the numeric representation. Preserve the
truthful numeric output while matching the handling used by other schema-invalid
values.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: QUIET
Plan: Pro Plus
Run ID: a2fe4ca5-1e0a-4403-9c88-3d21e3064e41
📒 Files selected for processing (16)
src/Cassandra.IntegrationTests/Core/StartupOptionsTests.cssrc/Cassandra.Tests/BuilderTests.cssrc/Cassandra.Tests/Cassandra.Tests.csprojsrc/Cassandra.Tests/ExecutionProfiles/RequestOptionsTests.cssrc/Cassandra.Tests/Requests/DriverConfigReporterTests.cssrc/Cassandra.Tests/Requests/StartupOptionsFactoryTests.cssrc/Cassandra.Tests/Requests/driver-config-report-v1.schema.jsonsrc/Cassandra.Tests/SocketOptionsTests.cssrc/Cassandra/Builder.cssrc/Cassandra/Configuration.cssrc/Cassandra/Connections/Connection.cssrc/Cassandra/Metadata.cssrc/Cassandra/Policies/DCAwareRoundRobinPolicy.cssrc/Cassandra/Requests/DriverConfigReporter.cssrc/Cassandra/SSLOptions.cssrc/Cassandra/SocketOptions.cs
Comment on lines
+355
to
+358
| // Through GetQueryAbortTimeout so that Timeout.Infinite survives the scaling; doubling it directly | ||
| // yields -2, which Task.Wait rejects. See Metadata.RefreshSchema. | ||
| return TaskHelper.WaitToComplete( | ||
| GetTableAsync(keyspace, tableName), Configuration.DefaultRequestOptions.GetQueryAbortTimeout(2)); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Prevent overflow when scaling the query timeout.
WithQueryTimeout(int.MaxValue) is valid. GetQueryAbortTimeout(2) then overflows to -2. Task.Wait rejects that value, so GetTable, GetMaterializedView, and RefreshSchema throw for a valid configuration. Clamp finite scaled values to int.MaxValue, or reject values that cannot be scaled. Add an int.MaxValue test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Cassandra/Metadata.cs` around lines 355 - 358, Prevent overflow in
GetQueryAbortTimeout when scaling a valid finite timeout such as int.MaxValue:
clamp scaled results to int.MaxValue (or reject unscalable values) while
preserving Timeout.Infinite. Ensure GetTable, GetMaterializedView, and
RefreshSchema pass only valid Task.Wait timeouts, and add coverage for
WithQueryTimeout(int.MaxValue) with scaling.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What ☑️
Stage 2 (the payload) of driver configuration reporting: replaces the stage-1
{"version":1}placeholder with the full
DRIVER_CONFIGreport — the effective configuration of the driver'sdefault execution profile plus the cluster's policies, serialized to the cross-driver JSON schema
shape.
This is phase 2 of 2. Stage 1 (#262) landed the plumbing and is merged, so this branch sits
directly on top of it and there is no stage-1 noise in the diff. Five commits, each building and
green on its own:
Expose whether the TLS options verify the server host name— an internal accessor onSSLOptions, plus moving the default cert-validation callback into a field so it can berecognized. Diagnostic only, nothing about how a connection is authenticated changes.
Expose whether the local datacenter was explicitly configured— an internal accessor onDCAwareRoundRobinPolicy, recorded in the constructor.Ship the normative driver config schema and its validator— the v1 schema verbatim as anembedded test resource, and
JsonSchema.Netin test scope. Vendored data and project plumbing,no logic.
Report the full driver configuration on the control connection— the report itself: thereporter, the reconciliation decisions, the schema-conformance tests.
Add integration tests for the full driver config report— over a real connection.Gated behind
Builder.WithDriverConfigReporting, which ships enabled (unchanged from stage 1).Turning it off suppresses only the
DRIVER_CONFIGblob —SESSION_IDrides on every connectionindependently of this flag, so "off" is not "zero change on the wire".
Fixes: https://scylladb.atlassian.net/browse/DRIVER-383