Driver config reporting — stage 2: full DRIVER_CONFIG report - #968
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 reports expanded default-profile configuration through control-connection Sequence Diagram(s)sequenceDiagram
participant StartupOptionsBuilder
participant ProtocolInitHandler
participant FeatureStore
participant DriverConfigReporter
StartupOptionsBuilder->>ProtocolInitHandler: provide stable SESSION_ID
ProtocolInitHandler->>FeatureStore: read sharding information
ProtocolInitHandler->>DriverConfigReporter: build control-connection DRIVER_CONFIG
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
Comment |
ffe0609 to
9a2f6ca
Compare
9a2f6ca to
c6f7ca3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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
`@core/src/main/java/com/datastax/oss/driver/api/core/config/DefaultDriverOption.java`:
- Around line 1178-1187: Make the reporting documentation backend-neutral across
DefaultDriverOption, TypedDriverOption, and reference.conf: replace
ScyllaDB-only wording with server-side terminology or explicitly document both
storage paths, system.clients for ScyllaDB and system_views.clients for
Cassandra 4.1. Update all three affected sites consistently without changing the
reporting behavior.
🪄 Autofix (Beta)
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: CHILL
Plan: Pro Plus
Run ID: ab5c16aa-71fa-4f9a-9659-654b6920ae50
📒 Files selected for processing (12)
core/src/main/java/com/datastax/oss/driver/api/core/config/DefaultDriverOption.javacore/src/main/java/com/datastax/oss/driver/api/core/config/TypedDriverOption.javacore/src/main/java/com/datastax/oss/driver/internal/core/channel/ProtocolInitHandler.javacore/src/main/java/com/datastax/oss/driver/internal/core/context/DefaultDriverConfigReporter.javacore/src/main/java/com/datastax/oss/driver/internal/core/context/DriverConfigReporter.javacore/src/main/java/com/datastax/oss/driver/internal/core/metadata/schema/queries/CassandraSchemaQueries.javacore/src/main/resources/reference.confcore/src/test/java/com/datastax/oss/driver/internal/core/channel/ChannelFactoryTestBase.javacore/src/test/java/com/datastax/oss/driver/internal/core/channel/ProtocolInitHandlerTest.javacore/src/test/java/com/datastax/oss/driver/internal/core/context/DefaultDriverConfigReporterTest.javaintegration-tests/src/test/java/com/datastax/oss/driver/core/config/DriverConfigReportingCcmIT.javaintegration-tests/src/test/java/com/datastax/oss/driver/core/config/DriverConfigReportingSimulacronIT.java
c6f7ca3 to
24062e9
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (4)
core/src/main/java/com/datastax/oss/driver/internal/core/context/DriverConfigReporter.java (1)
51-57: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueContract extension is consistent with implementation and callers.
The new
scyllaDbparam and its "only meaningful withreportDriverConfig" contract matchDefaultDriverConfigReporter.populateStartupOptionsandProtocolInitHandler's caller.One minor note for the future: this interface now has two adjacent
booleanparameters (reportDriverConfig,scyllaDb), which is a classic call-site readability/mix-up risk (e.g.populateStartupOptions(opts, true, false)reads ambiguously without named-parameter comments, as seen in the test file). Not blocking, but if a third flag is ever added, consider a small options value object instead.🤖 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 `@core/src/main/java/com/datastax/oss/driver/internal/core/context/DriverConfigReporter.java` around lines 51 - 57, The comment identifies no required code change; the current scyllaDb parameter and contract are consistent with the implementation and callers. Leave DriverConfigReporter.populateStartupOptions and its call sites unchanged, and only consider introducing an options value object if another boolean flag is added later.core/src/main/java/com/datastax/oss/driver/internal/core/channel/ProtocolInitHandler.java (1)
194-210: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCentralize the ScyllaDB predicate. Both this startup path and
CassandraSchemaQueries.shouldApplyUsingTimeout()key off the sameshardingInfo != nullsignal; a shared helper would keep control-plane reporting and schema-query behavior in sync.🤖 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 `@core/src/main/java/com/datastax/oss/driver/internal/core/channel/ProtocolInitHandler.java` around lines 194 - 210, Centralize the ScyllaDB detection based on getShardingInfo() != null in a shared helper, then update ProtocolInitHandler’s startup reporting and CassandraSchemaQueries.shouldApplyUsingTimeout() to use it. Preserve the existing featureStore population flow and behavior while ensuring both paths rely on the same predicate.core/src/test/java/com/datastax/oss/driver/internal/core/context/DefaultDriverConfigReporterTest.java (1)
136-145: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
controlno longer exercises the control-connection path.Both calls pass
reportDriverConfig=false, so the map namedcontrolis identical topool. Passingtruefor the control map keeps the test name honest and additionally proves the session id is stable when the config blob is built.♻️ Suggested tweak
- reporter.populateStartupOptions(control, false, false); + reporter.populateStartupOptions(control, /* reportDriverConfig= */ true, false); reporter.populateStartupOptions(pool, false, false);🤖 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 `@core/src/test/java/com/datastax/oss/driver/internal/core/context/DefaultDriverConfigReporterTest.java` around lines 136 - 145, Update should_use_a_stable_session_id_across_connections so the control map calls reporter.populateStartupOptions with reportDriverConfig=true, while keeping the pool call false and preserving the session ID equality assertion.integration-tests/src/test/java/com/datastax/oss/driver/core/config/DriverConfigReportingCcmIT.java (1)
145-169: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStage-2 payload assertion is copy-pasted across both integration tests. Both classes carry an identical
assertDriverConfigPayload(same Javadoc, same checks); every future stage-2 assertion has to be added twice and will silently drift otherwise.
integration-tests/src/test/java/com/datastax/oss/driver/core/config/DriverConfigReportingCcmIT.java#L145-L169: move this helper into a shared test utility (e.g. a package-privateDriverConfigReportAssertionsclass in this package) and call it from here.integration-tests/src/test/java/com/datastax/oss/driver/core/config/DriverConfigReportingSimulacronIT.java#L125-L149: delete the local copy and call the shared helper instead.🤖 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 `@integration-tests/src/test/java/com/datastax/oss/driver/core/config/DriverConfigReportingCcmIT.java` around lines 145 - 169, Extract the duplicated assertDriverConfigPayload helper into a package-private shared DriverConfigReportAssertions test utility, preserving its existing JSON parsing and stage-2 validation checks. In integration-tests/src/test/java/com/datastax/oss/driver/core/config/DriverConfigReportingCcmIT.java lines 145-169, replace the local helper with a call to the shared utility; in integration-tests/src/test/java/com/datastax/oss/driver/core/config/DriverConfigReportingSimulacronIT.java lines 125-149, delete the local copy and call the same utility.
🤖 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.
Nitpick comments:
In
`@core/src/main/java/com/datastax/oss/driver/internal/core/channel/ProtocolInitHandler.java`:
- Around line 194-210: Centralize the ScyllaDB detection based on
getShardingInfo() != null in a shared helper, then update ProtocolInitHandler’s
startup reporting and CassandraSchemaQueries.shouldApplyUsingTimeout() to use
it. Preserve the existing featureStore population flow and behavior while
ensuring both paths rely on the same predicate.
In
`@core/src/main/java/com/datastax/oss/driver/internal/core/context/DriverConfigReporter.java`:
- Around line 51-57: The comment identifies no required code change; the current
scyllaDb parameter and contract are consistent with the implementation and
callers. Leave DriverConfigReporter.populateStartupOptions and its call sites
unchanged, and only consider introducing an options value object if another
boolean flag is added later.
In
`@core/src/test/java/com/datastax/oss/driver/internal/core/context/DefaultDriverConfigReporterTest.java`:
- Around line 136-145: Update should_use_a_stable_session_id_across_connections
so the control map calls reporter.populateStartupOptions with
reportDriverConfig=true, while keeping the pool call false and preserving the
session ID equality assertion.
In
`@integration-tests/src/test/java/com/datastax/oss/driver/core/config/DriverConfigReportingCcmIT.java`:
- Around line 145-169: Extract the duplicated assertDriverConfigPayload helper
into a package-private shared DriverConfigReportAssertions test utility,
preserving its existing JSON parsing and stage-2 validation checks. In
integration-tests/src/test/java/com/datastax/oss/driver/core/config/DriverConfigReportingCcmIT.java
lines 145-169, replace the local helper with a call to the shared utility; in
integration-tests/src/test/java/com/datastax/oss/driver/core/config/DriverConfigReportingSimulacronIT.java
lines 125-149, delete the local copy and call the same utility.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5db2451e-3027-4740-bf80-230694676062
📒 Files selected for processing (15)
core/pom.xmlcore/src/main/java/com/datastax/oss/driver/api/core/config/DefaultDriverOption.javacore/src/main/java/com/datastax/oss/driver/api/core/config/TypedDriverOption.javacore/src/main/java/com/datastax/oss/driver/internal/core/channel/ProtocolInitHandler.javacore/src/main/java/com/datastax/oss/driver/internal/core/context/DefaultDriverConfigReporter.javacore/src/main/java/com/datastax/oss/driver/internal/core/context/DriverConfigReporter.javacore/src/main/java/com/datastax/oss/driver/internal/core/metadata/schema/queries/CassandraSchemaQueries.javacore/src/main/resources/reference.confcore/src/test/java/com/datastax/oss/driver/internal/core/channel/ChannelFactoryTestBase.javacore/src/test/java/com/datastax/oss/driver/internal/core/channel/ProtocolInitHandlerTest.javacore/src/test/java/com/datastax/oss/driver/internal/core/context/DefaultDriverConfigReporterTest.javacore/src/test/resources/config/driver-config-report-v1.schema.jsonintegration-tests/src/test/java/com/datastax/oss/driver/core/config/DriverConfigReportingCcmIT.javaintegration-tests/src/test/java/com/datastax/oss/driver/core/config/DriverConfigReportingSimulacronIT.javapom.xml
🚧 Files skipped from review as they are similar to previous changes (5)
- core/src/test/java/com/datastax/oss/driver/internal/core/channel/ChannelFactoryTestBase.java
- core/src/main/java/com/datastax/oss/driver/internal/core/metadata/schema/queries/CassandraSchemaQueries.java
- core/src/main/resources/reference.conf
- core/src/main/java/com/datastax/oss/driver/api/core/config/TypedDriverOption.java
- core/src/main/java/com/datastax/oss/driver/api/core/config/DefaultDriverOption.java
24062e9 to
d6ac737
Compare
|
@coderabbitai Thanks for the review — dispositions for the nitpicks, all pushed into the amended stage-2 commit:
Unrelated to the nitpicks but included in the same push: a TLS reporting correctness fix — |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== Relevant files =="
fd -t f . core/src/main integration-tests/src/test | rg 'DefaultDriverConfigReporter|DriverConfigReporting|SslEngineFactory|ProtocolInitHandler'
echo
echo "== Updated stable-session test and config reporting calls =="
rg -n -C 5 'should_use_a_stable_session_id_across_connections|populateStartupOptions\s*\(' \
core/src/test/java/com/datastax/oss/driver/internal/core/context/DefaultDriverConfigReporterTest.java \
core/src/main/java/com/datastax/oss/driver/internal/core/context/DefaultDriverConfigReporter.java \
core/src/main/java/com/datastax/oss/driver/internal/core/context/DriverConfigReporter.java \
core/src/main/java/com/datastax/oss/driver/internal/core/channel/ProtocolInitHandler.java
echo
echo "== Shared integration assertion helper and callers =="
rg -n -C 4 'class DriverConfigReportingAssertions|assertDriverConfigPayload\s*\(' \
integration-tests/src/test/java/com/datastax/oss/driver/core/config
echo
echo "== TLS hostname-verification reporting and SSL factory contract =="
rg -n -C 5 'hostname-verification|requireHostnameValidation\s*\(' \
core/src/main/java core/src/test/javaLength of output: 40821
✏️ Learnings added
You are interacting with an AI system. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
integration-tests/src/test/java/com/datastax/oss/driver/core/config/DriverConfigReportingCcmIT.java (1)
129-138: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftProve that the reporting row is the control connection.
This verifies cardinality, not identity: a regression that emits
DRIVER_CONFIGon one pool connection and omits it on the control connection still passes. Add a reliable control-connection marker/correlation before validating the payload, as the Simulacron test already does.🤖 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 `@integration-tests/src/test/java/com/datastax/oss/driver/core/config/DriverConfigReportingCcmIT.java` around lines 129 - 138, Update the reporting-row assertions in DriverConfigReportingCcmIT to correlate the DRIVER_CONFIG row with the control connection using the same reliable marker or correlation mechanism as the existing Simulacron test. Validate that the identified row is the control connection before calling assertDriverConfigPayload, while preserving the existing single-row cardinality check.core/src/test/java/com/datastax/oss/driver/internal/core/context/DefaultDriverConfigReporterTest.java (1)
321-352: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider single-argument override helpers to cut the
reporterWith(...)boilerplate.The 7-arg
reporterWith(defaults(map -> {}), mock(ExponentialReconnectionPolicy.class), mock(DefaultRetryPolicy.class), mock(NoSpeculativeExecutionPolicy.class), mock(DefaultLoadBalancingPolicy.class), mock(TimestampGenerator.class), Optional.empty())call is repeated ~15 times across this file, varying in exactly one argument. Thin wrappers (or a small builder) would make each test's intent obvious.♻️ Sketch
private DefaultDriverConfigReporter reporterWithReconnection(ReconnectionPolicy p) { return reporterWith( defaults(map -> {}), p, mock(DefaultRetryPolicy.class), mock(NoSpeculativeExecutionPolicy.class), mock(DefaultLoadBalancingPolicy.class), mock(TimestampGenerator.class), Optional.empty()); } // likewise reporterWithRetry / reporterWithSpecEx / reporterWithLb / reporterWithSsl🤖 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 `@core/src/test/java/com/datastax/oss/driver/internal/core/context/DefaultDriverConfigReporterTest.java` around lines 321 - 352, Reduce repeated seven-argument setup in DefaultDriverConfigReporterTest by adding thin single-argument reporterWith helper methods for the varying policy/configuration dependencies, including reconnection policy and the analogous retry, speculative execution, load-balancing, and SSL cases. Update the affected tests, such as should_report_constant_reconnection_policy and should_report_custom_reconnection_policy, to use the appropriate helper while preserving their existing mocks and 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.
Inline comments:
In `@core/src/test/resources/config/driver-config-report-v1.schema.json`:
- Around line 779-801: Update the consistency enum in the schema near the
consistency and serial-consistency properties to accept SERIAL and LOCAL_SERIAL
alongside the existing request consistency values. Keep the serial-consistency
property unchanged.
In
`@integration-tests/src/test/java/com/datastax/oss/driver/core/config/DriverConfigReportingAssertions.java`:
- Around line 29-43: Configure the shared OBJECT_MAPPER used by
assertDriverConfigPayload to enable
DeserializationFeature.FAIL_ON_TRAILING_TOKENS, ensuring readTree rejects valid
JSON followed by extra tokens while preserving the existing payload assertions.
---
Nitpick comments:
In
`@core/src/test/java/com/datastax/oss/driver/internal/core/context/DefaultDriverConfigReporterTest.java`:
- Around line 321-352: Reduce repeated seven-argument setup in
DefaultDriverConfigReporterTest by adding thin single-argument reporterWith
helper methods for the varying policy/configuration dependencies, including
reconnection policy and the analogous retry, speculative execution,
load-balancing, and SSL cases. Update the affected tests, such as
should_report_constant_reconnection_policy and
should_report_custom_reconnection_policy, to use the appropriate helper while
preserving their existing mocks and assertions.
In
`@integration-tests/src/test/java/com/datastax/oss/driver/core/config/DriverConfigReportingCcmIT.java`:
- Around line 129-138: Update the reporting-row assertions in
DriverConfigReportingCcmIT to correlate the DRIVER_CONFIG row with the control
connection using the same reliable marker or correlation mechanism as the
existing Simulacron test. Validate that the identified row is the control
connection before calling assertDriverConfigPayload, while preserving the
existing single-row cardinality check.
🪄 Autofix (Beta)
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: CHILL
Plan: Pro Plus
Run ID: 74f644f4-90dd-478c-b6ac-fecd6b72bac0
📒 Files selected for processing (20)
core/pom.xmlcore/src/main/java/com/datastax/oss/driver/api/core/config/DefaultDriverOption.javacore/src/main/java/com/datastax/oss/driver/api/core/config/TypedDriverOption.javacore/src/main/java/com/datastax/oss/driver/api/core/ssl/ProgrammaticSslEngineFactory.javacore/src/main/java/com/datastax/oss/driver/api/core/ssl/SslEngineFactory.javacore/src/main/java/com/datastax/oss/driver/internal/core/channel/ProtocolInitHandler.javacore/src/main/java/com/datastax/oss/driver/internal/core/context/DefaultDriverConfigReporter.javacore/src/main/java/com/datastax/oss/driver/internal/core/context/DriverConfigReporter.javacore/src/main/java/com/datastax/oss/driver/internal/core/metadata/schema/queries/CassandraSchemaQueries.javacore/src/main/java/com/datastax/oss/driver/internal/core/ssl/DefaultSslEngineFactory.javacore/src/main/java/com/datastax/oss/driver/internal/core/ssl/SniSslEngineFactory.javacore/src/main/resources/reference.confcore/src/test/java/com/datastax/oss/driver/internal/core/channel/ChannelFactoryTestBase.javacore/src/test/java/com/datastax/oss/driver/internal/core/channel/ProtocolInitHandlerTest.javacore/src/test/java/com/datastax/oss/driver/internal/core/context/DefaultDriverConfigReporterTest.javacore/src/test/resources/config/driver-config-report-v1.schema.jsonintegration-tests/src/test/java/com/datastax/oss/driver/core/config/DriverConfigReportingAssertions.javaintegration-tests/src/test/java/com/datastax/oss/driver/core/config/DriverConfigReportingCcmIT.javaintegration-tests/src/test/java/com/datastax/oss/driver/core/config/DriverConfigReportingSimulacronIT.javapom.xml
🚧 Files skipped from review as they are similar to previous changes (4)
- core/src/main/java/com/datastax/oss/driver/api/core/config/TypedDriverOption.java
- core/src/main/java/com/datastax/oss/driver/internal/core/metadata/schema/queries/CassandraSchemaQueries.java
- core/src/main/resources/reference.conf
- core/src/main/java/com/datastax/oss/driver/api/core/config/DefaultDriverOption.java
d6ac737 to
5e69715
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
core/src/main/java/com/datastax/oss/driver/internal/core/channel/ProtocolInitHandler.java (1)
194-203: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCentralize the ScyllaDB detection check
getShardingInfo() != nullis used here and again inCassandraSchemaQueries.shouldApplyUsingTimeout(). A shared helper would keep driver-config reporting andUSING TIMEOUTgating aligned if the detection logic changes later.🤖 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 `@core/src/main/java/com/datastax/oss/driver/internal/core/channel/ProtocolInitHandler.java` around lines 194 - 203, Centralize the ScyllaDB detection currently implemented by getShardingInfo() != null into a shared helper, then update the ProtocolInitHandler flow and CassandraSchemaQueries.shouldApplyUsingTimeout() to use it. Preserve the existing featureStore null handling and ensure both driver-config reporting and USING TIMEOUT gating rely on the same detection logic.
🤖 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.
Nitpick comments:
In
`@core/src/main/java/com/datastax/oss/driver/internal/core/channel/ProtocolInitHandler.java`:
- Around line 194-203: Centralize the ScyllaDB detection currently implemented
by getShardingInfo() != null into a shared helper, then update the
ProtocolInitHandler flow and CassandraSchemaQueries.shouldApplyUsingTimeout() to
use it. Preserve the existing featureStore null handling and ensure both
driver-config reporting and USING TIMEOUT gating rely on the same detection
logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: af57b7b6-6e4e-44fd-9609-e06c113aa08b
📒 Files selected for processing (20)
core/pom.xmlcore/src/main/java/com/datastax/oss/driver/api/core/config/DefaultDriverOption.javacore/src/main/java/com/datastax/oss/driver/api/core/config/TypedDriverOption.javacore/src/main/java/com/datastax/oss/driver/api/core/ssl/ProgrammaticSslEngineFactory.javacore/src/main/java/com/datastax/oss/driver/api/core/ssl/SslEngineFactory.javacore/src/main/java/com/datastax/oss/driver/internal/core/channel/ProtocolInitHandler.javacore/src/main/java/com/datastax/oss/driver/internal/core/context/DefaultDriverConfigReporter.javacore/src/main/java/com/datastax/oss/driver/internal/core/context/DriverConfigReporter.javacore/src/main/java/com/datastax/oss/driver/internal/core/metadata/schema/queries/CassandraSchemaQueries.javacore/src/main/java/com/datastax/oss/driver/internal/core/ssl/DefaultSslEngineFactory.javacore/src/main/java/com/datastax/oss/driver/internal/core/ssl/SniSslEngineFactory.javacore/src/main/resources/reference.confcore/src/test/java/com/datastax/oss/driver/internal/core/channel/ChannelFactoryTestBase.javacore/src/test/java/com/datastax/oss/driver/internal/core/channel/ProtocolInitHandlerTest.javacore/src/test/java/com/datastax/oss/driver/internal/core/context/DefaultDriverConfigReporterTest.javacore/src/test/resources/config/driver-config-report-v1.schema.jsonintegration-tests/src/test/java/com/datastax/oss/driver/core/config/DriverConfigReportingAssertions.javaintegration-tests/src/test/java/com/datastax/oss/driver/core/config/DriverConfigReportingCcmIT.javaintegration-tests/src/test/java/com/datastax/oss/driver/core/config/DriverConfigReportingSimulacronIT.javapom.xml
🚧 Files skipped from review as they are similar to previous changes (4)
- core/src/main/java/com/datastax/oss/driver/api/core/config/TypedDriverOption.java
- core/src/main/java/com/datastax/oss/driver/internal/core/metadata/schema/queries/CassandraSchemaQueries.java
- core/src/main/java/com/datastax/oss/driver/api/core/config/DefaultDriverOption.java
- core/src/main/resources/reference.conf
Fills in the full DRIVER_CONFIG JSON report in the approved v2
cross-driver schema shape, replacing the stage-1 {"version":1}
placeholder. All groups are populated from Configuration and
Policies on each control-connection init.
Adds public getters to DCAwareRoundRobinPolicy and
RackAwareRoundRobinPolicy needed to report node-location-preference
and dc-failover, and makes PagingOptimizingLoadBalancingPolicy
implement ChainableLoadBalancingPolicy so the reporter can unwrap the
LB policy Cluster.Manager wraps at runtime.
Adds a JSON-Schema conformance test suite (mirroring the 4.x sibling
PR scylladb#968): the normative schema is shipped as a test resource and
validated via com.networknt:json-schema-validator (pinned to 1.5.x,
the last line still targeting Java 8), covering every discriminated-
union branch and optional group the 3.x reporter can emit, plus a
negative test proving additionalProperties=false is enforced.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Fills in the full DRIVER_CONFIG JSON report in the approved v2
cross-driver schema shape, replacing the stage-1 {"version":1}
placeholder. All groups are populated from Configuration and
Policies on each control-connection init.
Adds public getters to DCAwareRoundRobinPolicy and
RackAwareRoundRobinPolicy needed to report node-location-preference
and dc-failover, and makes PagingOptimizingLoadBalancingPolicy
implement ChainableLoadBalancingPolicy so the reporter can unwrap the
LB policy Cluster.Manager wraps at runtime.
Adds a JSON-Schema conformance test suite (mirroring the 4.x sibling
PR scylladb#968): the normative schema is shipped as a test resource and
validated via com.networknt:json-schema-validator (pinned to 1.5.x,
the last line still targeting Java 8), covering every discriminated-
union branch and optional group the 3.x reporter can emit, plus a
negative test proving additionalProperties=false is enforced.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Fills in the full DRIVER_CONFIG JSON report in the approved v2
cross-driver schema shape, replacing the stage-1 {"version":1}
placeholder. All groups are populated from Configuration and
Policies on each control-connection init.
Adds public getters to DCAwareRoundRobinPolicy and
RackAwareRoundRobinPolicy needed to report node-location-preference
and dc-failover, and makes PagingOptimizingLoadBalancingPolicy
implement ChainableLoadBalancingPolicy so the reporter can unwrap the
LB policy Cluster.Manager wraps at runtime.
Adds a JSON-Schema conformance test suite (mirroring the 4.x sibling
PR scylladb#968): the normative schema is shipped as a test resource and
validated via com.networknt:json-schema-validator (pinned to 1.5.x,
the last line still targeting Java 8), covering every discriminated-
union branch and optional group the 3.x reporter can emit, plus a
negative test proving additionalProperties=false is enforced.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Fills in the full DRIVER_CONFIG JSON report in the approved v2
cross-driver schema shape, replacing the stage-1 {"version":1}
placeholder. All groups are populated from Configuration and Policies
when the report is built, i.e. once per Cluster as it initializes.
Adds public getters to DCAwareRoundRobinPolicy and
RackAwareRoundRobinPolicy needed to report node-location-preference
and dc-failover, and makes PagingOptimizingLoadBalancingPolicy
implement ChainableLoadBalancingPolicy so the reporter can unwrap the
LB policy Cluster.Manager wraps at runtime.
Adds a JSON-Schema conformance test suite (mirroring the 4.x sibling
PR scylladb#968): the normative schema is shipped as a test resource and
validated via com.networknt:json-schema-validator (pinned to 1.5.x,
the last line still targeting Java 8), covering every discriminated-
union branch and optional group the 3.x reporter can emit, plus a
negative test proving additionalProperties=false is enforced.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
5e69715 to
cfda714
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
core/src/test/java/com/datastax/oss/driver/internal/core/context/DefaultDriverConfigReporterTest.java (1)
930-966: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider a small builder for the reporter fixtures.
The 7-argument and 8-argument
reporterWithcalls repeat across about twenty tests, and each call varies only one argument. A builder that starts from the default policy set and overrides one collaborator would remove that repetition and make each test state its single variable.Example shape:
private final class ReporterBuilder { private DriverExecutionProfile profile = defaults(map -> {}); private ReconnectionPolicy reconnection = mock(ExponentialReconnectionPolicy.class); // ... remaining collaborators with the same defaults as defaultsReporter() ReporterBuilder reconnection(ReconnectionPolicy p) { this.reconnection = p; return this; } DefaultDriverConfigReporter build() { /* wire the mock context */ } }Each test then reads
builder().reconnection(mock(ConstantReconnectionPolicy.class)).build().🤖 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 `@core/src/test/java/com/datastax/oss/driver/internal/core/context/DefaultDriverConfigReporterTest.java` around lines 930 - 966, Refactor the repeated reporter fixture setup around the overloaded reporterWith methods into a small ReporterBuilder that initializes the same defaults as defaultsReporter() and exposes fluent overrides for individual collaborators, including the programmatic local datacenter. Update the affected tests to build reporters by overriding only the variable under test, while preserving the existing mock context wiring and behavior.core/src/main/java/com/datastax/oss/driver/internal/core/context/DefaultDriverConfigReporter.java (1)
152-152: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider package-private visibility for
buildJson.
DefaultDriverConfigReporterTestis in the same package,com.datastax.oss.driver.internal.core.context. Package-private visibility therefore supports the test override without adding a subclass extension point that the javadoc must then qualify with thread-safety caveats.♻️ Proposed change
- protected String buildJson(boolean scyllaDb) { + String buildJson(boolean scyllaDb) {If a production subclass hook is intended, keep
protectedand disregard this suggestion.🤖 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 `@core/src/main/java/com/datastax/oss/driver/internal/core/context/DefaultDriverConfigReporter.java` at line 152, Change the buildJson method in DefaultDriverConfigReporter from protected to package-private visibility, allowing DefaultDriverConfigReporterTest to override it within the same package without exposing a production subclass extension point.
🤖 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 `@core/src/main/java/com/datastax/oss/driver/api/core/config/OptionsMap.java`:
- Line 403: Restore opt-in driver configuration reporting by setting
TypedDriverOption.DRIVER_CONFIG_REPORTING_ENABLED to false in OptionsMap. Update
DriverConfigReportingSimulacronIT at lines 53-60 and 145-160 to assert and
enable reporting explicitly as needed, and update upgrade_guide/README.md lines
40-44 to document that reporting is disabled by default.
In
`@core/src/main/java/com/datastax/oss/driver/internal/core/context/DriverConfigReporter.java`:
- Around line 23-33: Change driver-config reporting defaults from enabled to
disabled across OptionsMap.fillWithDriverDefaults and
DefaultDriverConfigReporter, then update the corresponding documentation and
tests to reflect false as the default. Preserve explicit opt-in behavior,
ensuring default sessions do not send the DRIVER_CONFIG startup option.
In
`@integration-tests/src/test/java/com/datastax/oss/driver/core/config/DriverConfigReportingCcmIT.java`:
- Around line 133-142: Update the assertions in DriverConfigReportingCcmIT to
identify the DRIVER_CONFIG row by matching its connection local address and port
against the control connection, using the existing control-connection details
and clientOptions helpers. Preserve the single-row and payload assertions, but
ensure a pooled connection cannot satisfy the test.
In `@upgrade_guide/README.md`:
- Around line 42-44: Update the fenced configuration block in the README to
specify the HOCON language identifier, changing the opening fence to use hocon
while preserving the existing configuration content.
---
Nitpick comments:
In
`@core/src/main/java/com/datastax/oss/driver/internal/core/context/DefaultDriverConfigReporter.java`:
- Line 152: Change the buildJson method in DefaultDriverConfigReporter from
protected to package-private visibility, allowing
DefaultDriverConfigReporterTest to override it within the same package without
exposing a production subclass extension point.
In
`@core/src/test/java/com/datastax/oss/driver/internal/core/context/DefaultDriverConfigReporterTest.java`:
- Around line 930-966: Refactor the repeated reporter fixture setup around the
overloaded reporterWith methods into a small ReporterBuilder that initializes
the same defaults as defaultsReporter() and exposes fluent overrides for
individual collaborators, including the programmatic local datacenter. Update
the affected tests to build reporters by overriding only the variable under
test, while preserving the existing mock context wiring and behavior.
🪄 Autofix (Beta)
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: CHILL
Plan: Pro Plus
Run ID: e40d8974-45a6-4285-bc5b-2d6d30c334dc
📒 Files selected for processing (26)
core/pom.xmlcore/src/main/java/com/datastax/oss/driver/api/core/config/DefaultDriverOption.javacore/src/main/java/com/datastax/oss/driver/api/core/config/OptionsMap.javacore/src/main/java/com/datastax/oss/driver/api/core/config/TypedDriverOption.javacore/src/main/java/com/datastax/oss/driver/api/core/ssl/ProgrammaticSslEngineFactory.javacore/src/main/java/com/datastax/oss/driver/api/core/ssl/SslEngineFactory.javacore/src/main/java/com/datastax/oss/driver/internal/core/channel/ProtocolInitHandler.javacore/src/main/java/com/datastax/oss/driver/internal/core/context/DefaultDriverConfigReporter.javacore/src/main/java/com/datastax/oss/driver/internal/core/context/DefaultDriverContext.javacore/src/main/java/com/datastax/oss/driver/internal/core/context/DriverConfigReporter.javacore/src/main/java/com/datastax/oss/driver/internal/core/context/StartupOptionsBuilder.javacore/src/main/java/com/datastax/oss/driver/internal/core/metadata/schema/queries/CassandraSchemaQueries.javacore/src/main/java/com/datastax/oss/driver/internal/core/ssl/DefaultSslEngineFactory.javacore/src/main/java/com/datastax/oss/driver/internal/core/ssl/SniSslEngineFactory.javacore/src/main/resources/reference.confcore/src/test/java/com/datastax/dse/driver/internal/core/context/DseStartupOptionsBuilderTest.javacore/src/test/java/com/datastax/oss/driver/internal/core/channel/ChannelFactoryTestBase.javacore/src/test/java/com/datastax/oss/driver/internal/core/channel/ProtocolInitHandlerTest.javacore/src/test/java/com/datastax/oss/driver/internal/core/context/DefaultDriverConfigReporterTest.javacore/src/test/java/com/datastax/oss/driver/internal/core/context/StartupOptionsBuilderTest.javacore/src/test/resources/config/driver-config-report-v1.schema.jsonintegration-tests/src/test/java/com/datastax/oss/driver/core/config/DriverConfigReportingAssertions.javaintegration-tests/src/test/java/com/datastax/oss/driver/core/config/DriverConfigReportingCcmIT.javaintegration-tests/src/test/java/com/datastax/oss/driver/core/config/DriverConfigReportingSimulacronIT.javapom.xmlupgrade_guide/README.md
🚧 Files skipped from review as they are similar to previous changes (13)
- core/src/main/java/com/datastax/oss/driver/api/core/config/TypedDriverOption.java
- core/src/main/java/com/datastax/oss/driver/api/core/config/DefaultDriverOption.java
- core/src/main/java/com/datastax/oss/driver/internal/core/ssl/DefaultSslEngineFactory.java
- core/src/main/java/com/datastax/oss/driver/api/core/ssl/ProgrammaticSslEngineFactory.java
- integration-tests/src/test/java/com/datastax/oss/driver/core/config/DriverConfigReportingAssertions.java
- core/src/main/java/com/datastax/oss/driver/internal/core/metadata/schema/queries/CassandraSchemaQueries.java
- core/pom.xml
- core/src/main/java/com/datastax/oss/driver/internal/core/ssl/SniSslEngineFactory.java
- core/src/main/java/com/datastax/oss/driver/api/core/ssl/SslEngineFactory.java
- core/src/test/resources/config/driver-config-report-v1.schema.json
- pom.xml
- core/src/main/resources/reference.conf
- core/src/test/java/com/datastax/oss/driver/internal/core/channel/ChannelFactoryTestBase.java
cfda714 to
312b522
Compare
|
Dispositions for the two nitpicks:
|
|
On the two nitpicks from the last CodeRabbit pass: |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 34 out of 35 changed files in this pull request and generated no new comments.
Suppressed comments (4)
core/src/main/java/com/datastax/oss/driver/internal/core/context/DefaultDriverConfigReporter.java:940
trimmedToNullchanges the reported DC/rack even though the load-balancing helpers use the configured string verbatim (OptionalLocalDcHelper.java:71-79andOptionalLocalRackHelper.java:34-38). For example," dc1 "matches nodc1nodes at runtime but is reported as"dc1"; an empty/whitespace value can also be turned intodc-autoeven though the policy treats it as explicitly configured and does not infer. Preserve non-empty values exactly, and omit an unrepresentable empty value without treating it as absent for inference.
String trimmed = s.trim();
core/src/main/java/com/datastax/oss/driver/internal/core/context/DefaultDriverConfigReporter.java:626
- This reports
truefrom the raw max-remote-nodes option even whenBasicLoadBalancingPolicyhas no local DC. In that modemaybeAddDcFailoverreturns immediately and all nodes are treated as local (BasicLoadBalancingPolicy.java:577-578, 724-726), so the option is inert and both node-preference groups are omitted, yet the report claims fallback to non-preferred nodes is active. Gate this value on an effective node preference/local DC and cover the DC-agnostic Basic policy with a positive failover setting.
n.put(
"fallback-to-non-preferred-nodes",
config.getInt(DefaultDriverOption.LOAD_BALANCING_DC_FAILOVER_MAX_NODES_PER_REMOTE_DC, 0)
> 0);
core/src/main/java/com/datastax/oss/driver/internal/core/context/DefaultDriverConfigReporter.java:333
- The class documentation now correctly lists two required fields with no schema-valid form (
in-flight.maxandquery.defaults.consistency), but this comment still says there is only one. Keep the count consistent.
// clamped — the one field left with no schema-valid form, see the class javadoc.
core/src/test/java/com/datastax/oss/driver/internal/core/channel/ProtocolInitHandlerTest.java:341
- This comment is stale: the reporter intentionally reads only whether sharding info is present and no longer reports pool sizing or consumes the shard count. Update it to describe the unwrapping/backend signal being tested.
// The reporter receives the node-level info unwrapped from ConnectionShardingInfo, carrying the
// SCYLLA_NR_SHARDS count above — which is what it reports the connection pool's sizing from.
Fills in the full DRIVER_CONFIG JSON report in the normative cross-driver
schema shape, replacing the stage-1 {"version":1} placeholder. All groups
are populated from Configuration and Policies when the report is built,
i.e. once per Cluster as it initializes.
Everything hangs off three groups. connection carries the connect/read
timeouts, the per-connection request capacity, the pool, the socket
options, the reconnection policy and -- only when TLS is on -- tls.
control-plane carries the system-query and schema-agreement timeouts.
query carries the per-request defaults plus the three policies acting on
a query: retry, load-balancing (with the node preference beside it) and,
when configured, speculative-execution.
The schema also has an optional connection.node-preference, for a driver
that scopes its connection pools independently of its query routing. 3.x
has no such second knob -- one LoadBalancingPolicy decides both, since
distance(Host) governs whether a host is pooled at all -- so the
preference is reported once, under the policy it derives from, rather
than duplicated.
token-aware is the only built-in load balancing shape the schema defines,
so every other built-in policy -- a bare DCAwareRoundRobinPolicy,
RoundRobinPolicy, WhiteListPolicy -- is reported as custom with its class
name, which identifies it but carries none of the normalized flags; its
datacenter and rack still show up in query.load-balancing.node-preference.
A token-aware chain reports load-distribution from its replica ordering
(RANDOM, the 3.x default, is "shuffle"; TOPOLOGICAL is "replica-set";
NEUTRAL keeps the child's plan order, so "round-robin").
fallback-to-non-preferred-nodes is true whenever the policy can reach a
node outside the preference reported beside it. For DCAwareRoundRobin
that means used-hosts-per-remote-DC, since the preference is the
datacenter. RackAwareRoundRobin reports a rack, and the other racks of
its local datacenter are outside that yet are the second tier of every
query plan -- distance() returns REMOTE, not IGNORED, for them -- so it
is always true there, remote datacenter hosts or not.
adaptive-ordering has no "enabled" flag and cannot carry an empty signal
list, so it is reported only when a LatencyAwarePolicy is in the chain --
latency being the only runtime observation a 3.x policy can reorder
candidates on. tls likewise has no "enabled" flag: the group's presence
is what says TLS is on.
Where a configured value falls outside what the schema can express, an
optional key or group is omitted rather than emitted as a value the
schema rejects: a disabled connect timeout, a disabled read timeout (all
three of connection.read, control-plane.queries.system.timeout
.client-side-ms and query.defaults.request), a negative SO_LINGER, a
non-positive socket buffer size, an unbounded page size, and a default
serial consistency level that is not serial -- QueryOptions, unlike
Statement, does not check that one. Two optional bounds are omitted for
the opposite reason -- 3.x has no such bound to report at all:
connection.reconnection.policy.max-attempts, since its reconnection
policies retry forever (the maxAttempts field ExponentialReconnection
Policy carries is an overflow guard on the doubling, not a give-up
bound: past it nextDelayMs() keeps returning maxDelayMs), and
query.retry.policy.max-retries, since no
single number describes a 3.x retry policy. Both built-ins are
parameterless singletons, and while they stop after one attempt on a
read timeout, a write timeout or an unavailable error -- all three
sharing one counter, so one retry between them rather than one each --
onRequestError leaves nbRetry unread and keeps trying the next host
until the query plan runs out. Which of the two applies is decided per
statement rather than by configuration: RequestHandler only consults
onRequestError and onWriteTimeout for an idempotent statement, so the
same policy bounds a non-idempotent request at one retry and an
idempotent one at the length of the query plan, and setIdempotent
overrides the reported query.defaults.idempotence per statement.
Two more keys are omitted for a third reason -- the schema admits only a
boolean, and 3.x cannot observe which one applies.
query.defaults.client-timestamps is false for ServerSideTimestamp
Generator, whose next() always returns Long.MIN_VALUE, and true for an
AbstractMonotonicTimestampGenerator, which never can; any other
generator makes that a per-call decision, so whether timestamps are
assigned client-side is not a property of the configuration at all.
connection.tls.hostname-verification is true for SniSSLOptions, the
driver's only setEndpointIdentificationAlgorithm call, and omitted for
every other SSLOptions, which builds its engine from a user SSLContext
or hands the whole handler to Netty. Both keys are documented as absent
exactly when the behavior is unknown, which is this case. The tls group
can therefore be empty -- its presence is still what reports TLS is on.
Omission is not always available, so these required keys are left in the
one state that is accurate:
- connection.requests.orphaned.max has no 3.x equivalent to report at
all. A request the driver stopped waiting for keeps its stream
identifier until the response arrives, with no configurable bound and
no connection replacement, so the key is omitted -- which its
required-ness then rejects. This is the one violation every report
carries.
- connection.requests.in-flight.max must be positive, while
PoolingOptions also accepts 0. Only PoolingOptions.UNSET falls back to
a protocol default, so a limit of 0 an operator set deliberately is
not reported as 1024.
- query.speculative-execution.policy.percentile is bounded to 0..100
exclusive, while PercentileSpeculativeExecutionPolicy accepts a
percentile of 0.
Such a value is reported as-is and the limitation is documented on the
class: the reporter neither fabricates an in-range value -- which would
misreport a setting an operator may have chosen on purpose, or a policy
3.x does not implement -- nor drops the whole report over one field.
Recorded as a cross-driver schema gap, to be fixed the way
control-plane.schema.agreement.timeout-ms already admits 0.
QueryOptions.setConsistencyLevel now rejects null -- a behavior change
to a public setter. Every query needs a consistency level, so a null
default already failed any statement that did not set one of its own:
SessionManager falls back to it for every request, and
CBUtil.writeConsistencyLevel then dereferences it to write the frame.
That turned a schema-required key into a missing one for a
configuration that could never work. setSerialConsistencyLevel is
deliberately left as it is: the schema makes serial-consistency
optional, so a null there is faithfully reported as an omission rather
than as a missing required key. The reporter keeps omitting a
null it is handed anyway: the field is private, so only a QueryOptions
subclass overriding the getter can still produce one, and letting it
through would throw and cost the whole report rather than one key.
Adds the public getters the report needs: local DC/rack, their explicit
flags and used-hosts-per-remote-DC on DCAwareRoundRobinPolicy, the same
minus used-hosts-per-remote-DC on RackAwareRoundRobinPolicy, replica
ordering on TokenAwarePolicy, and max-executions plus the delay or
percentile on the two built-in speculative execution policies -- whose
parameters are immutable and land in the schema's range exactly, so
they are reported as constant/percentile rather than as custom. Also
makes PagingOptimizingLoadBalancingPolicy implement
ChainableLoadBalancingPolicy so the reporter can unwrap the LB policy
Cluster.Manager wraps at runtime.
in-flight.max needs a fallback because PoolingOptions is still UNSET
when the report is built: the protocol version is only negotiated once
the control connection is up. The default row is resolved with the same
walk PoolingOptions.setProtocolVersion applies -- the highest DEFAULTS
key not above the version -- driven by the version the user pinned with
withProtocolVersion when they pinned one, and by v3 otherwise, that
being the lowest version ScyllaDB negotiates and the reference row for
everything above it. DEFAULTS holds only v1 and v3 rows, so a cluster
pinned to v2 is sized from v1's 128 rather than v3's 1024, and pinning
is the one part of negotiation knowable at report time.
Caps the report at 32KiB of UTF-8 (MAX_DRIVER_CONFIG_LENGTH), matching
the 4.x sibling PR scylladb#968, gocql scylladb#964 and csharp-driver scylladb#262. Beyond
cross-driver parity this is a correctness fix: CBUtil.writeString
writes each STARTUP value with a 16-bit length prefix and no bounds
check, so a value over 65535 bytes truncates the prefix modulo 65536
while still appending the whole body -- a corrupt frame and a failed
handshake, and not something the fail-safe try/catch can contain since
nothing throws. Parts of the report are user-supplied and unbounded
(DC/rack names, consistency levels, custom policy class names). Over
the limit means WARN and no DRIVER_CONFIG.
Hardens the other two ways reporting could break a connection rather
than merely fail to report:
- The fail-safe catch also covers InternalError, since customPolicy()
calls getClass().getSimpleName() on arbitrary user policy objects
(documented JDK edge case for certain synthetic classes). Not a bare
Error, so OutOfMemoryError/StackOverflowError still surface.
- The load balancing policy chain walk is bounded at 16 policies and
shared by both callers. It follows getChildPolicy() on arbitrary user
policies, so a cyclic chain used to spin forever on the Cluster
initialization path -- the one failure mode the try/catch cannot
contain, because it hangs rather than throws.
A custom load balancing policy is now named after the policy the user
configured rather than PagingOptimizingLoadBalancingPolicy. Cluster
.Manager wraps every session's policy in that internal class, and it is
the outermost element of the chain, so every custom policy was reported
as {"type":"custom","name":"PagingOptimizingLoadBalancingPolicy"}. An
anonymous policy class falls back to its binary name, since it has no
simple name and the schema requires a non-empty one.
Adds a JSON-Schema conformance test suite (mirroring the 4.x sibling
PR scylladb#968): the normative schema block is shipped verbatim as a test
resource -- design-doc revision v5, whose report version field is still
1 -- and validated via com.networknt:json-schema-validator (1.5.x,
the last line still targeting Java 8), covering every discriminated-
union branch and optional group the 3.x reporter can emit. Since one
required key has no value to report, the assertion is that a report
violates the schema in exactly the documented ways and no other, with
a test naming the gap and a negative test proving
additionalProperties=false is enforced.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
bd539a1 to
4ddf4ca
Compare
| if (clientSideMs > 0) { | ||
| timeout.put("client-side-ms", clientSideMs); | ||
| } | ||
| if (scyllaDb) { |
There was a problem hiding this comment.
Report the configured server-side timeout without backend gating. METADATA_SCHEMA_REQUEST_TIMEOUT is known regardless of peer detection, so emit server-side-ms whenever it is configured and positive. Its client-side schema-query timeout role should also get a matching V1 field.
There was a problem hiding this comment.
Ungated in dda5f10: reported whenever positive, on any backend. That was the reporter's only backend-conditional value, so the NodeShardingInfo argument threaded from ProtocolInitHandler is gone too. Two asks for the spec: server-side-ms wants the "reports configuration intent" clause pool.shard-aware.enabled already carries, since on Cassandra nothing enforces it. And the two halves of your comment interact — once queries.schema exists, METADATA_SCHEMA_REQUEST_TIMEOUT belongs there rather than under queries.system, so this field would migrate. Happy to implement both the moment the document lands.
There was a problem hiding this comment.
Same on the queries.schema half: the vendored schema is a carbon copy of the document, so the sibling has to appear there before anything here can emit it — queries is additionalProperties: false today. Ready to resync and implement as soon as it lands.
There was a problem hiding this comment.
Same here: v6 is vendored as of 9de9af1 and adds no queries.schema sibling, so METADATA_SCHEMA_REQUEST_TIMEOUT still has nowhere to move and queries remains additionalProperties: false. Ready to resync and wire up the payload the moment it lands.
| @Nullable | ||
| private NodeLocation nodeLocation(DriverExecutionProfile config, LoadBalancingPolicy policy) { | ||
| String configuredDc = | ||
| trimmedToNull(context.getLocalDatacenter(DriverExecutionProfile.DEFAULT_NAME)); |
There was a problem hiding this comment.
Do not normalize node-location values only in the reporter. Runtime DC/rack helpers consume configured strings verbatim, so padded or whitespace values are changed or omitted here despite being schema-valid. Normalize runtime too, or serialize the exact active values.
There was a problem hiding this comment.
Fixed in 7bfc53c for the padded case — you're right, and my own justification never covered it: nonEmptyString is minLength: 1, so " dc1 " was always valid to emit and trimming hid the typo the report exists to expose. Blank still maps to "unset", where the schema genuinely leaves no alternative. Normalizing the runtime helpers I'd rather not do here — it changes routing, in a reporting PR.
| || policyClass == DcInferringLoadBalancingPolicy.class | ||
| || policyClass == DseLoadBalancingPolicy.class | ||
| || policyClass == DseDcInferringLoadBalancingPolicy.class) { | ||
| avoidsSlowReplicas = |
There was a problem hiding this comment.
Determine adaptive ordering from the active policy, not reloadable config. DefaultLoadBalancingPolicy latches this flag at construction, so after reload the group can be omitted while runtime ordering remains enabled, or reported while disabled. Expose and read the latched value.
There was a problem hiding this comment.
Fixed in 97f883c, via a new DefaultLoadBalancingPolicy.isAvoidingSlowReplicas(). Did the same for fallback-to-non-preferred-nodes, whose max-nodes-per-remote-dc term BasicLoadBalancingPolicy latches identically — those were the last two profile reads for latched state, so the class javadoc's exception list is now empty. Both pinned by reload-divergence tests.
| } | ||
| n.set("connect", connect); | ||
| n.set("requests", requests(config)); | ||
| ObjectNode pool = OBJECT_MAPPER.createObjectNode(); |
There was a problem hiding this comment.
Report pool sizes. connection.pool currently emits only shard awareness even though local.size and remote.size are always configured and consumed by ChannelPool. Add both values to the V1 schema and payload; additive fields do not require a major-version bump.
There was a problem hiding this comment.
Agreed, and this closes the flag I raised when desired-connections-count was dropped. Blocked here only by the document: $defs/connection-pool is additionalProperties: false and this branch ships the schema block verbatim. Two things to settle in the shape — whether 0 is representable (positiveInteger reproduces the objection you raised against the old field), and that ChannelPool.initialize() ceil-divides the configured size across shards, so local.size = 1 on a 4-shard node opens four connections; the number Java would report is not the connection count. Say which and I'll wire both up.
There was a problem hiding this comment.
One thing to make explicit, since it decides who does what: core/src/test/resources/config/driver-config-report-v1.schema.json is a byte-for-byte copy of the design document's normative block. Adding local.size / remote.size here first would fork the contract and make our conformance suite pass against a schema no other driver has — so the keys land in the document, on your side, and I resync and wire up the payload. Same shape question stands: whether 0 is representable, and the per-shard division.
There was a problem hiding this comment.
Resynced to the v6 document in 9de9af1 — its only delta was making connection.requests.orphaned optional, so local.size/remote.size are still not in the schema and this stays blocked on the document. The two shape questions stand: whether 0 is representable, and that ChannelPool.initialize() ceil-divides the configured size across shards.
4ddf4ca to
7bfc53c
Compare
Fills in the full DRIVER_CONFIG JSON report in the normative cross-driver
schema shape, replacing the stage-1 {"version":1} placeholder. All groups
are populated from Configuration and Policies when the report is built,
i.e. once per Cluster as it initializes.
Everything hangs off three groups. connection carries the connect/read
timeouts, the per-connection request capacity, the pool, the socket
options, the reconnection policy and -- only when TLS is on -- tls.
control-plane carries the system-query and schema-agreement timeouts.
query carries the per-request defaults plus the three policies acting on
a query: retry, load-balancing (with the node preference beside it) and,
when configured, speculative-execution.
The schema reports the node preference in two places and 3.x fills both
from the same policy chain: query.load-balancing.node-preference for what
a query is routed by, connection.node-preference for the part of the
cluster the driver holds connections to. One LoadBalancingPolicy decides
both, since distance(Host) governs whether a host is pooled at all, so
the connection key carries the datacenter half alone. A rack-aware
policy's distance() returns REMOTE, never IGNORED, for a local-datacenter
host in another rack, so those hosts are still pooled and the rack scopes
no pooling at all; the datacenter does, a host outside the preferred one
being IGNORED unless the policy is configured to use hosts there, and an
ignored host gets no pool.
token-aware is the only built-in load balancing shape the schema defines,
so every other built-in policy -- a bare DCAwareRoundRobinPolicy,
RoundRobinPolicy, WhiteListPolicy -- is reported as custom with its class
name, which identifies it but carries none of the normalized flags; its
datacenter and rack still show up in query.load-balancing.node-preference.
A token-aware chain reports load-distribution from its replica ordering
(RANDOM, the 3.x default, is "shuffle"; TOPOLOGICAL is "replica-set";
NEUTRAL keeps the child's plan order, so "round-robin").
fallback-to-non-preferred-nodes is true whenever the policy can reach a
node outside the preference reported beside it. For DCAwareRoundRobin
that means used-hosts-per-remote-DC, since the preference is the
datacenter. RackAwareRoundRobin reports a rack, and the other racks of
its local datacenter are outside that yet are the second tier of every
query plan -- distance() returns REMOTE, not IGNORED, for them -- so it
is always true there, remote datacenter hosts or not.
adaptive-ordering has no "enabled" flag and cannot carry an empty signal
list, so it is reported only when a LatencyAwarePolicy is in the chain --
latency being the only runtime observation a 3.x policy can reorder
candidates on. tls likewise has no "enabled" flag: the group's presence
is what says TLS is on.
Where a configured value falls outside what the schema can express, an
optional key or group is omitted rather than emitted as a value the
schema rejects: a disabled connect timeout, a disabled read timeout (all
three of connection.read, control-plane.queries.system.timeout
.client-side-ms and query.defaults.request), a negative SO_LINGER, a
non-positive socket buffer size, an unbounded page size, and a default
serial consistency level that is not serial -- QueryOptions, unlike
Statement, does not check that one. Two optional bounds are omitted for
the opposite reason -- 3.x has no such bound to report at all:
connection.reconnection.policy.max-attempts, since its reconnection
policies retry forever (the maxAttempts field ExponentialReconnection
Policy carries is an overflow guard on the doubling, not a give-up
bound: past it nextDelayMs() keeps returning maxDelayMs), and
query.retry.policy.max-retries, since no
single number describes a 3.x retry policy. Both built-ins are
parameterless singletons, and while they stop after one attempt on a
read timeout, a write timeout or an unavailable error -- all three
sharing one counter, so one retry between them rather than one each --
onRequestError leaves nbRetry unread and keeps trying the next host
until the query plan runs out. Which of the two applies is decided per
statement rather than by configuration: RequestHandler only consults
onRequestError and onWriteTimeout for an idempotent statement, so the
same policy bounds a non-idempotent request at one retry and an
idempotent one at the length of the query plan, and setIdempotent
overrides the reported query.defaults.idempotence per statement.
Two more keys are omitted for a third reason -- the schema admits only a
boolean, and 3.x cannot observe which one applies.
query.defaults.client-timestamps is false for ServerSideTimestamp
Generator, whose next() always returns Long.MIN_VALUE, and true for an
AbstractMonotonicTimestampGenerator, which never can; any other
generator makes that a per-call decision, so whether timestamps are
assigned client-side is not a property of the configuration at all.
connection.tls.hostname-verification is true for SniSSLOptions, the
driver's only setEndpointIdentificationAlgorithm call, and omitted for
every other SSLOptions, which builds its engine from a user SSLContext
or hands the whole handler to Netty. Both keys are documented as absent
exactly when the behavior is unknown, which is this case. The tls group
can therefore be empty -- its presence is still what reports TLS is on.
Omission is not always available, so these required keys are left in the
one state that is accurate:
- connection.requests.orphaned.max has no 3.x equivalent to report at
all. A request the driver stopped waiting for keeps its stream
identifier until the response arrives, with no configurable bound and
no connection replacement, so the key is omitted -- which its
required-ness then rejects. This is the one violation every report
carries.
- connection.requests.in-flight.max must be positive, while
PoolingOptions also accepts 0. Only PoolingOptions.UNSET falls back to
a protocol default, so a limit of 0 an operator set deliberately is
not reported as 1024.
- query.speculative-execution.policy.percentile is bounded to 0..100
exclusive, while PercentileSpeculativeExecutionPolicy accepts a
percentile of 0.
Such a value is reported as-is and the limitation is documented on the
class: the reporter neither fabricates an in-range value -- which would
misreport a setting an operator may have chosen on purpose, or a policy
3.x does not implement -- nor drops the whole report over one field.
Recorded as a cross-driver schema gap, to be fixed the way
control-plane.schema.agreement.timeout-ms already admits 0.
QueryOptions.setConsistencyLevel now rejects null -- a behavior change
to a public setter. Every query needs a consistency level, so a null
default already failed any statement that did not set one of its own:
SessionManager falls back to it for every request, and
CBUtil.writeConsistencyLevel then dereferences it to write the frame.
That turned a schema-required key into a missing one for a
configuration that could never work. setSerialConsistencyLevel is
deliberately left as it is: the schema makes serial-consistency
optional, so a null there is faithfully reported as an omission rather
than as a missing required key. The reporter keeps omitting a
null it is handed anyway: the field is private, so only a QueryOptions
subclass overriding the getter can still produce one, and letting it
through would throw and cost the whole report rather than one key.
Adds the public getters the report needs: local DC/rack, their explicit
flags and used-hosts-per-remote-DC on DCAwareRoundRobinPolicy, the same
minus used-hosts-per-remote-DC on RackAwareRoundRobinPolicy, replica
ordering on TokenAwarePolicy, and max-executions plus the delay or
percentile on the two built-in speculative execution policies -- whose
parameters are immutable and land in the schema's range exactly, so
they are reported as constant/percentile rather than as custom. Also
makes PagingOptimizingLoadBalancingPolicy implement
ChainableLoadBalancingPolicy so the reporter can unwrap the LB policy
Cluster.Manager wraps at runtime.
in-flight.max needs a fallback because PoolingOptions is still UNSET
when the report is built: the protocol version is only negotiated once
the control connection is up. The default row is resolved with the same
walk PoolingOptions.setProtocolVersion applies -- the highest DEFAULTS
key not above the version -- driven by the version the user pinned with
withProtocolVersion when they pinned one, and by v3 otherwise, that
being the lowest version ScyllaDB negotiates and the reference row for
everything above it. DEFAULTS holds only v1 and v3 rows, so a cluster
pinned to v2 is sized from v1's 128 rather than v3's 1024, and pinning
is the one part of negotiation knowable at report time.
Caps the report at 32KiB of UTF-8 (MAX_DRIVER_CONFIG_LENGTH), matching
the 4.x sibling PR scylladb#968, gocql scylladb#964 and csharp-driver scylladb#262. Beyond
cross-driver parity this is a correctness fix: CBUtil.writeString
writes each STARTUP value with a 16-bit length prefix and no bounds
check, so a value over 65535 bytes truncates the prefix modulo 65536
while still appending the whole body -- a corrupt frame and a failed
handshake, and not something the fail-safe try/catch can contain since
nothing throws. Parts of the report are user-supplied and unbounded
(DC/rack names, consistency levels, custom policy class names). Over
the limit means WARN and no DRIVER_CONFIG.
Hardens the other two ways reporting could break a connection rather
than merely fail to report:
- The fail-safe catch also covers InternalError, since customPolicy()
calls getClass().getSimpleName() on arbitrary user policy objects
(documented JDK edge case for certain synthetic classes). Not a bare
Error, so OutOfMemoryError/StackOverflowError still surface.
- The load balancing policy chain walk is bounded at 16 policies and
shared by both callers. It follows getChildPolicy() on arbitrary user
policies, so a cyclic chain used to spin forever on the Cluster
initialization path -- the one failure mode the try/catch cannot
contain, because it hangs rather than throws.
A custom load balancing policy is now named after the policy the user
configured rather than PagingOptimizingLoadBalancingPolicy. Cluster
.Manager wraps every session's policy in that internal class, and it is
the outermost element of the chain, so every custom policy was reported
as {"type":"custom","name":"PagingOptimizingLoadBalancingPolicy"}. An
anonymous policy class falls back to its binary name, since it has no
simple name and the schema requires a non-empty one.
Adds a JSON-Schema conformance test suite (mirroring the 4.x sibling
PR scylladb#968): the normative schema block is shipped verbatim as a test
resource -- design-doc revision v5, whose report version field is still
1 -- and validated via com.networknt:json-schema-validator (1.5.x,
the last line still targeting Java 8), covering every discriminated-
union branch and optional group the 3.x reporter can emit. Since one
required key has no value to report, the assertion is that a report
violates the schema in exactly the documented ways and no other, with
a test naming the gap and a negative test proving
additionalProperties=false is enforced.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Fills in the full DRIVER_CONFIG JSON report in the normative cross-driver
schema shape, replacing the stage-1 {"version":1} placeholder. All groups
are populated from Configuration and Policies when the report is built,
i.e. once per Cluster as it initializes.
Everything hangs off three groups. connection carries the connect/read
timeouts, the per-connection request capacity, the pool, the socket
options, the reconnection policy and -- only when TLS is on -- tls.
control-plane carries the system-query and schema-agreement timeouts.
query carries the per-request defaults plus the three policies acting on
a query: retry, load-balancing (with the node preference beside it) and,
when configured, speculative-execution.
The schema reports the node preference in two places and 3.x fills both
from the same policy chain: query.load-balancing.node-preference for what
a query is routed by, connection.node-preference for the part of the
cluster the driver holds connections to. One LoadBalancingPolicy decides
both, since distance(Host) governs whether a host is pooled at all, so
the connection key carries the datacenter half alone. A rack-aware
policy's distance() returns REMOTE, never IGNORED, for a local-datacenter
host in another rack, so those hosts are still pooled and the rack scopes
no pooling at all; the datacenter does, a host outside the preferred one
being IGNORED unless the policy is configured to use hosts there, and an
ignored host gets no pool.
token-aware is the only built-in load balancing shape the schema defines,
so every other built-in policy -- a bare DCAwareRoundRobinPolicy,
RoundRobinPolicy, WhiteListPolicy -- is reported as custom with its class
name, which identifies it but carries none of the normalized flags; its
datacenter and rack still show up in query.load-balancing.node-preference.
A token-aware chain reports load-distribution from its replica ordering
(RANDOM, the 3.x default, is "shuffle"; TOPOLOGICAL is "replica-set";
NEUTRAL keeps the child's plan order, so "round-robin").
fallback-to-non-preferred-nodes is true whenever the policy can reach a
node outside the preference reported beside it. For DCAwareRoundRobin
that means used-hosts-per-remote-DC, since the preference is the
datacenter. RackAwareRoundRobin reports a rack, and the other racks of
its local datacenter are outside that yet are the second tier of every
query plan -- distance() returns REMOTE, not IGNORED, for them -- so it
is always true there, remote datacenter hosts or not.
adaptive-ordering has no "enabled" flag and cannot carry an empty signal
list, so it is reported only when a LatencyAwarePolicy is in the chain --
latency being the only runtime observation a 3.x policy can reorder
candidates on. tls likewise has no "enabled" flag: the group's presence
is what says TLS is on.
Where a configured value falls outside what the schema can express, an
optional key or group is omitted rather than emitted as a value the
schema rejects: a disabled connect timeout, a disabled read timeout (all
three of connection.read, control-plane.queries.system.timeout
.client-side-ms and query.defaults.request), a negative SO_LINGER, a
non-positive socket buffer size, an unbounded page size, and a default
serial consistency level that is not serial -- QueryOptions, unlike
Statement, does not check that one. Two optional bounds are omitted for
the opposite reason -- 3.x has no such bound to report at all:
connection.reconnection.policy.max-attempts, since its reconnection
policies retry forever (the maxAttempts field ExponentialReconnection
Policy carries is an overflow guard on the doubling, not a give-up
bound: past it nextDelayMs() keeps returning maxDelayMs), and
query.retry.policy.max-retries, since no
single number describes a 3.x retry policy. Both built-ins are
parameterless singletons, and while they stop after one attempt on a
read timeout, a write timeout or an unavailable error -- all three
sharing one counter, so one retry between them rather than one each --
onRequestError leaves nbRetry unread and keeps trying the next host
until the query plan runs out. Which of the two applies is decided per
statement rather than by configuration: RequestHandler only consults
onRequestError and onWriteTimeout for an idempotent statement, so the
same policy bounds a non-idempotent request at one retry and an
idempotent one at the length of the query plan, and setIdempotent
overrides the reported query.defaults.idempotence per statement.
Two more keys are omitted for a third reason -- the schema admits only a
boolean, and 3.x cannot observe which one applies.
query.defaults.client-timestamps is false for ServerSideTimestamp
Generator, whose next() always returns Long.MIN_VALUE, and true for an
AbstractMonotonicTimestampGenerator, which never can; any other
generator makes that a per-call decision, so whether timestamps are
assigned client-side is not a property of the configuration at all.
connection.tls.hostname-verification is true for SniSSLOptions, the
driver's only setEndpointIdentificationAlgorithm call, and omitted for
every other SSLOptions, which builds its engine from a user SSLContext
or hands the whole handler to Netty. Both keys are documented as absent
exactly when the behavior is unknown, which is this case. The tls group
can therefore be empty -- its presence is still what reports TLS is on.
Omission is not always available, so these required keys are left in the
one state that is accurate:
- connection.requests.orphaned.max has no 3.x equivalent to report at
all. A request the driver stopped waiting for keeps its stream
identifier until the response arrives, with no configurable bound and
no connection replacement, so the key is omitted -- which its
required-ness then rejects. This is the one violation every report
carries.
- connection.requests.in-flight.max must be positive, while
PoolingOptions also accepts 0. Only PoolingOptions.UNSET falls back to
a protocol default, so a limit of 0 an operator set deliberately is
not reported as 1024.
- query.speculative-execution.policy.percentile is bounded to 0..100
exclusive, while PercentileSpeculativeExecutionPolicy accepts a
percentile of 0.
Such a value is reported as-is and the limitation is documented on the
class: the reporter neither fabricates an in-range value -- which would
misreport a setting an operator may have chosen on purpose, or a policy
3.x does not implement -- nor drops the whole report over one field.
Recorded as a cross-driver schema gap, to be fixed the way
control-plane.schema.agreement.timeout-ms already admits 0.
QueryOptions.setConsistencyLevel now rejects null -- a behavior change
to a public setter. Every query needs a consistency level, so a null
default already failed any statement that did not set one of its own:
SessionManager falls back to it for every request, and
CBUtil.writeConsistencyLevel then dereferences it to write the frame.
That turned a schema-required key into a missing one for a
configuration that could never work. setSerialConsistencyLevel is
deliberately left as it is: the schema makes serial-consistency
optional, so a null there is faithfully reported as an omission rather
than as a missing required key. The reporter keeps omitting a
null it is handed anyway: the field is private, so only a QueryOptions
subclass overriding the getter can still produce one, and letting it
through would throw and cost the whole report rather than one key.
Adds the public getters the report needs: local DC/rack, their explicit
flags and used-hosts-per-remote-DC on DCAwareRoundRobinPolicy, the same
minus used-hosts-per-remote-DC on RackAwareRoundRobinPolicy, replica
ordering on TokenAwarePolicy, and max-executions plus the delay or
percentile on the two built-in speculative execution policies -- whose
parameters are immutable and land in the schema's range exactly, so
they are reported as constant/percentile rather than as custom. Also
makes PagingOptimizingLoadBalancingPolicy implement
ChainableLoadBalancingPolicy so the reporter can unwrap the LB policy
Cluster.Manager wraps at runtime.
in-flight.max needs a fallback because PoolingOptions is still UNSET
when the report is built: the protocol version is only negotiated once
the control connection is up. The default row is resolved with the same
walk PoolingOptions.setProtocolVersion applies -- the highest DEFAULTS
key not above the version -- driven by the version the user pinned with
withProtocolVersion when they pinned one, and by v3 otherwise, that
being the lowest version ScyllaDB negotiates and the reference row for
everything above it. DEFAULTS holds only v1 and v3 rows, so a cluster
pinned to v2 is sized from v1's 128 rather than v3's 1024, and pinning
is the one part of negotiation knowable at report time.
Caps the report at 32KiB of UTF-8 (MAX_DRIVER_CONFIG_LENGTH), matching
the 4.x sibling PR scylladb#968, gocql scylladb#964 and csharp-driver scylladb#262. Beyond
cross-driver parity this is a correctness fix: CBUtil.writeString
writes each STARTUP value with a 16-bit length prefix and no bounds
check, so a value over 65535 bytes truncates the prefix modulo 65536
while still appending the whole body -- a corrupt frame and a failed
handshake, and not something the fail-safe try/catch can contain since
nothing throws. Parts of the report are user-supplied and unbounded
(DC/rack names, consistency levels, custom policy class names). Over
the limit means WARN and no DRIVER_CONFIG.
Hardens the other two ways reporting could break a connection rather
than merely fail to report:
- The fail-safe catch also covers InternalError, since customPolicy()
calls getClass().getSimpleName() on arbitrary user policy objects
(documented JDK edge case for certain synthetic classes). Not a bare
Error, so OutOfMemoryError/StackOverflowError still surface.
- The load balancing policy chain walk is bounded at 16 policies and
shared by both callers. It follows getChildPolicy() on arbitrary user
policies, so a cyclic chain used to spin forever on the Cluster
initialization path -- the one failure mode the try/catch cannot
contain, because it hangs rather than throws.
A custom load balancing policy is now named after the policy the user
configured rather than PagingOptimizingLoadBalancingPolicy. Cluster
.Manager wraps every session's policy in that internal class, and it is
the outermost element of the chain, so every custom policy was reported
as {"type":"custom","name":"PagingOptimizingLoadBalancingPolicy"}. An
anonymous policy class falls back to its binary name, since it has no
simple name and the schema requires a non-empty one.
Adds a JSON-Schema conformance test suite (mirroring the 4.x sibling
PR scylladb#968): the normative schema block is shipped verbatim as a test
resource -- design-doc revision v5, whose report version field is still
1 -- and validated via com.networknt:json-schema-validator (1.5.x,
the last line still targeting Java 8), covering every discriminated-
union branch and optional group the 3.x reporter can emit. Since one
required key has no value to report, the assertion is that a report
violates the schema in exactly the documented ways and no other, with
a test naming the gap and a negative test proving
additionalProperties=false is enforced.
The report is now built behind a guard at its one call site, so a
classpath without Jackson cannot break connecting. Stage 1 made
jackson-core/jackson-databind required compile-scope dependencies of
driver-core, and DefaultDriverConfigReporter holds an ObjectMapper in a
static field: exclude jackson-databind and merely initializing that class
raises NoClassDefFoundError. That is an Error, raised while initializing
the class rather than from any method it declares, so neither the
reporter's own fail-safe nor its caller could contain it, and it happens
on the Cluster initialization path -- so a classpath that merely lacks an
optional serializer went from "the report is skipped" to "no connection
can be established", the inverse of the invariant this class is written
around. Connection.Factory.buildDriverConfigReport now catches
LinkageError -- not just NoClassDefFoundError, so a version-mismatched
Jackson surfacing as ExceptionInInitializerError is covered too, where
probing one class name would pass and still fail -- and reports nothing,
logging at WARN since reporting ships enabled and nobody opted in. Same
fallback SnappyCompressor already applies for its own optional library,
and no contradiction with buildReport() deliberately not catching bare
Error: that is about report building never masking a real JVM failure,
this is a call site tolerating a missing optional dependency.
Both node preference slots are documented as an approximation once a
wrapper sits above the policy they were read from. HostFilterPolicy
.distance() -- and so WhiteListPolicy's, which extends it -- returns
IGNORED for any host failing its predicate, including one inside the
reported datacenter, and a custom chainable policy computes distance()
itself and need honor nothing below it. The configured datacenter is
reported anyway, on the grounds that hiding one the operator really did
set is worse, and the asymmetry is deliberate: nothing is inferred on a
third party's behalf, but what was configured is passed through. The
restriction has nowhere to go, the built-in shape having no room for a
wrapper and fromDCWhiteList collapsing its datacenters into an opaque
Predicate<Host>.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Motivation: The DRIVER_CONFIG report is consumed as a cross-driver contract, so it has to be checked against the normative schema rather than against this driver's own idea of the shape. Nothing in 3.x could do that. Modifications: Vendors the schema block verbatim from the design doc, revision v5 -- whose report version field is still 1 -- as a test resource. This is the same resource the 4.x sibling PR scylladb#968 ships, so the two drivers are held to one document. Validation runs on com.networknt:json-schema-validator, pinned to the 1.5.x line because it is the last minor line still targeting Java 8. The conformance tests assert on this validator's exact ValidationMessage wording, so a bump may need those strings updated; that is noted on the version property in the parent pom. Result: The resource and the dependency are in place. Nothing consumes them yet -- the reporter and the conformance tests that validate against them follow in later commits. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…on time
Motivation:
Stage 1 wired up DRIVER_CONFIG and sent a {"version":1} placeholder. This
fills in the report itself, in the normative cross-driver schema shape, so an
operator can see from the server what a client is actually configured with.
Modifications:
The report is built once per Cluster as it initializes, from Configuration
and Policies, and hangs off three groups. connection carries the connect/read
timeouts, the per-connection request capacity, the pool, the socket options,
the reconnection policy and -- only when TLS is on -- tls. control-plane
carries the system-query and schema-agreement timeouts. query carries the
per-request defaults plus the three policies acting on a query: retry,
load-balancing (with the node preference beside it) and, when configured,
speculative-execution.
The schema reports the node preference in two places and 3.x fills both from
the same policy chain: query.load-balancing.node-preference for what a query
is routed by, connection.node-preference for the part of the cluster the
driver holds connections to. One LoadBalancingPolicy decides both, since
distance(Host) governs whether a host is pooled at all, so the connection key
carries the datacenter half alone. A rack-aware policy's distance() returns
REMOTE, never IGNORED, for a local-datacenter host in another rack, so the
rack scopes no pooling at all; the datacenter does, a host outside the
preferred one being IGNORED unless the policy is configured to use hosts
there, and an ignored host gets no pool.
token-aware is the only built-in load balancing shape the schema defines, so
every other built-in policy -- a bare DCAwareRoundRobinPolicy,
RoundRobinPolicy, WhiteListPolicy -- is reported as custom with its class
name, which identifies it but carries none of the normalized flags. A
token-aware chain reports load-distribution from its replica ordering
(RANDOM, the 3.x default, is "shuffle"; TOPOLOGICAL is "replica-set";
NEUTRAL keeps the child's plan order, so "round-robin"). A custom policy is
named after the policy the user configured, not after
PagingOptimizingLoadBalancingPolicy, which Cluster.Manager wraps every
session's policy in and which is the outermost element of the chain; an
anonymous class falls back to its binary name, having no simple name where
the schema requires a non-empty one.
fallback-to-non-preferred-nodes is true whenever the policy can reach a node
outside the preference reported beside it. For DCAwareRoundRobin that means
used-hosts-per-remote-DC, since the preference is the datacenter.
RackAwareRoundRobin reports a rack, and the other racks of its local
datacenter are outside that yet are the second tier of every query plan, so
it is always true there, remote datacenter hosts or not.
in-flight.max needs a fallback because PoolingOptions is still UNSET when the
report is built: the protocol version is only negotiated once the control
connection is up. The default row is resolved with the same walk
PoolingOptions.setProtocolVersion applies -- the highest DEFAULTS key not
above the version -- driven by the version the user pinned with
withProtocolVersion when they pinned one, and by v3 otherwise, that being the
lowest version ScyllaDB negotiates. DEFAULTS holds only v1 and v3 rows, so a
cluster pinned to v2 is sized from v1's 128 rather than v3's 1024, and
pinning is the one part of negotiation knowable at report time.
Three ways reporting could break a connection rather than merely fail to
report are closed off:
- The report is capped at 32KiB of UTF-8 (MAX_DRIVER_CONFIG_LENGTH), matching
the 4.x sibling PR scylladb#968, gocql scylladb#964 and csharp-driver scylladb#262. Beyond
cross-driver parity this is a correctness fix: CBUtil.writeString writes
each STARTUP value with a 16-bit length prefix and no bounds check, so a
value over 65535 bytes truncates the prefix modulo 65536 while still
appending the whole body -- a corrupt frame and a failed handshake, and not
something the fail-safe try/catch can contain since nothing throws. Parts
of the report are user-supplied and unbounded (DC/rack names, consistency
levels, custom policy class names). Over the limit means WARN and no
DRIVER_CONFIG.
- The fail-safe catch also covers InternalError, since customPolicy() calls
getClass().getSimpleName() on arbitrary user policy objects (a documented
JDK edge case for certain synthetic classes). Not a bare Error, so
OutOfMemoryError and StackOverflowError still surface.
- The load balancing chain walk is bounded at 16 policies and shared by both
callers. It follows getChildPolicy() on arbitrary user policies, so a
cyclic chain used to spin forever on the Cluster initialization path -- the
one failure mode a try/catch cannot contain, because it hangs rather than
throws.
Result:
Every group the reporter can emit is validated against the normative schema
shipped earlier in this series, covering each discriminated-union branch and
optional group, with a negative test proving additionalProperties=false is
enforced.
Where a configured value falls outside what the schema can express, an
optional key or group is omitted rather than emitted as a value the schema
rejects; where a required key has no accurate value to carry, it is left in
the one state that is accurate and the limitation is documented on the class.
The assertion is therefore that a report violates the schema in exactly the
documented ways and no other. The PR description catalogues each omission and
the three schema gaps recorded for the cross-driver document.
Both node preference slots are an approximation once a wrapper sits above the
policy they were read from. HostFilterPolicy.distance() -- and so
WhiteListPolicy's, which extends it -- returns IGNORED for any host failing
its predicate, including one inside the reported datacenter, and a custom
chainable policy computes distance() itself and need honor nothing below it.
The configured datacenter is reported anyway, on the grounds that hiding one
the operator really did set is worse. The asymmetry is deliberate: nothing is
inferred on a third party's behalf, but what was configured is passed
through. The restriction has nowhere to go, the built-in shape having no room
for a wrapper and fromDCWhiteList collapsing its datacenters into an opaque
Predicate<Host>.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2fdf28f to
ee8e0be
Compare
|
Force-pushed
Why now: this repo rebase-merges rather than squashes — #982's three commits landed on
Regrouping by soft-reset to the merge base keeps only the net tree, so all three pairs disappear by construction, along with the ~14 review-round One deviation from a clean 9: the default-on flip stayed inside the reporter commit instead of becoming its own. The flip and the documentation of what is being enabled are a single javadoc block in Verified per commit: all eight compile clean on JDK 11, 3904 core unit tests pass at head, The previous head |
Motivation: The DRIVER_CONFIG report is consumed as a cross-driver contract, so it has to be checked against the normative schema rather than against this driver's own idea of the shape. Modifications: Vendors the schema block verbatim from the design doc, revision v5 -- whose report version field is still 1 -- as a test resource. This is the same resource the 3.x sibling PR scylladb#974 ships, so the two drivers are held to one document. Validation runs on com.networknt:json-schema-validator, pinned to the 1.5.x line because it is the last minor line still targeting Java 8. The conformance tests assert on this validator's exact ValidationMessage wording, so a bump may need those strings updated; that is noted on the version property in the parent pom. Result: The resource and the dependency are in place. Nothing consumes them yet -- the reporter and the conformance tests that validate against them follow in later commits. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Motivation: Stage 1 had the config reporter emit both startup options, so SESSION_ID was governed by advanced.driver-config-reporting.enabled and disappeared when reporting was turned off. That conflates two different things: SESSION_ID is what lets the server group a session's connections, and it is useful whether or not the driver also describes its configuration. The cross-driver review settled this the same way for gocql and 3.x -- SESSION_ID is reported no matter what. Modifications: SESSION_ID moves to StartupOptionsBuilder, beside the other innate startup options, and is sent on every connection unconditionally. It is generated lazily rather than in a field initializer, mirroring clientId; DefaultDriverContext builds the startup options exactly once per session (LazyReference), which is what makes the value stable across all of the session's connections, including reconnects. It is deliberately not derived from the user-settable, Insights-oriented CLIENT_ID, so that it is guaranteed unique per session as a grouping key requires. DriverConfigReporter accordingly narrows from populateStartupOptions(options, reportDriverConfig) to populateControlConnectionOptions(options): the reporter now has one job, and the control-connection-only decision moves to the caller in ProtocolInitHandler. Result: Turning driver config reporting off no longer leaves the wire unchanged -- SESSION_ID is still sent. That is documented on the option itself. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…nwrap Motivation: Two values the configuration report needs were computed inline where only one caller could reach them. The effective max-orphan-requests is not always the configured advanced.connection.max-orphan-requests: that option has to stay below advanced.connection.max-requests-per-connection, and a value that does not is silently corrected to a quarter of it. The correction lived inside ChannelFactory's channel-initialization block, so a second caller could only duplicate it -- and a report that duplicated it slightly differently would claim a limit the connection was not built with. Separately, ShardingInfo.ConnectionShardingInfo carries a shardId specific to one connection, which node-level and session-level callers have to unwrap past every time. Modifications: - ChannelFactory.effectiveMaxOrphanRequests(maxRequestsPerConnection, maxOrphanRequests) is now a static method, and the initialization block calls it. The warning still logs the configured value and the corrected one rather than recomputing either. - ProtocolFeatureStore.getNodeShardingInfo() does the unwrap once, returning null when the server advertised no sharding information -- which is the driver's own proxy check for "this is not ScyllaDB". DriverChannel delegates to it, and CassandraSchemaQueries, the other caller of that proxy check, gets a punctuation fix on the comment describing it. Result: Pure extraction, no behavior change. One implementation of each, so the report cannot drift from what the connection was actually built with. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Motivation: The report has to describe what a session is actually running with, and for several components that is not what the configuration currently says. The built-in policies read their options once, at construction, and keep them in private fields; after a configuration reload the profile holds new values while the policy still runs on the old ones. Reading the profile would describe a policy that is not in effect. The SSL engine factories hold the same kind of state -- whether they configure the engine to validate host names -- and the built-in one latches it from the configuration the same way. Modifications: Adds accessors for that state, so the report can read the instance rather than the config: - BasicLoadBalancingPolicy and DefaultLoadBalancingPolicy: the datacenter and rack the policy actually resolved, and whether adaptive ordering is on -- which DefaultLoadBalancingPolicy also latches at construction. - ConstantReconnectionPolicy and ConstantSpeculativeExecutionPolicy: the parameters they were built with. - JdkSslHandlerFactory: the SslEngineFactory it wraps, so host name validation is read from the factory actually in use rather than from a possibly unused one configured alongside it. A context that overrides buildSslHandlerFactory() may wrap a factory of its own choosing, in which case the configured one is never consulted on the connection path. - DefaultSslEngineFactory and ProgrammaticSslEngineFactory: whether they require host name validation. Deliberately on the classes rather than on the SslEngineFactory interface. An accessor there would need a default, and there is no honest default: an arbitrary factory can neither be assumed to validate host names nor be assumed not to, so the default answer would misdescribe a security control for every implementation that never considered the question. The report names the factories it recognizes and says nothing about the rest. Two of the load-balancing accessors widen from package-private to public; that is noted on them. Result: Pure widening -- no behavior changes. Every accessor returns state the component had already computed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Motivation:
Stage 1 wired up DRIVER_CONFIG and sent a {"version":1} placeholder. This
fills in the report itself, in the normative cross-driver schema shape, so an
operator can see from the server what a client is actually configured with --
and turns reporting on by default, since a diagnostic nobody enables is a
diagnostic nobody has during an incident.
Modifications:
The report is built once per session, from the default execution profile and
the policies actually in force, and hangs off three groups. connection
carries the connect/read timeouts, the per-connection request capacity, the
pool, the socket options, the reconnection policy and -- only when TLS is on
-- tls. control-plane carries the system-query and schema-agreement timeouts.
query carries the per-request defaults plus the three policies acting on a
query: retry, load-balancing (with the node preference beside it) and, when
configured, speculative-execution.
Values are read from the running components rather than from the profile
wherever the two can disagree, using the accessors added earlier in this
series: a policy latches its options at construction, so after a
configuration reload the profile holds values the policy is not using. The
same applies to host name validation, which is read from the SslEngineFactory
the active SslHandlerFactory wraps rather than from the configured one, and
to the effective orphaned-request limit, which is the corrected value the
connection was actually built with.
Where the driver cannot know an answer, the key is left out rather than
guessed: an SslEngineFactory or TimestampGenerator that reports
Optional.empty() produces no field at all, which is what the schema asks for.
The default flips to true in reference.conf and OptionsMap, and the option's
documentation is rewritten in the same edit -- it described a placeholder
payload and a default of false, both of which this commit changes, so the
prose and the value move together.
Reporting stays best-effort and off the critical path: the report is capped
at 32 KiB of UTF-8, and a report that cannot be built, or that exceeds the
cap, is skipped with a warning rather than allowed to interfere with
connecting.
Result:
Every group the reporter can emit is validated against the normative schema
shipped earlier in this series, covering each discriminated-union branch and
optional group, with a negative test proving additionalProperties=false is
enforced.
Where a configured value falls outside what the schema can express, an
optional key or group is omitted rather than emitted as a value the schema
rejects. The PR description catalogues each omission and the schema gaps
recorded against the cross-driver document.
The node preference is an approximation once a custom chainable policy sits
above the one it was read from, since such a policy computes distance()
itself and need honor nothing below it. The configured datacenter is reported
anyway, on the grounds that hiding one the operator really did set is worse;
that is documented on the field.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…bsent Motivation: Stage 1 made jackson-core and jackson-databind required compile-scope dependencies of core, and DefaultDriverConfigReporter holds an ObjectMapper in a static field. The driver declares Jackson as required but documents that it can be excluded when unused (manual/core/integration), so on such a classpath merely *linking* the default implementation raises a NoClassDefFoundError. That is an Error, raised while initializing the class rather than thrown from any method it declares, so neither the reporter's own fail-safe nor its caller in ProtocolInitHandler could contain it -- and it happens on the connection initialization path. A documented, supported configuration went from "reporting is skipped" to "no connection can be established", the inverse of the invariant this class is written around. Modifications: DefaultDriverContext.buildDriverConfigReporter() now checks for Jackson and returns a NoopDriverConfigReporter when it is absent. Choosing which implementation to instantiate is the only place the check can live, since anything later has already touched the class. Logged unconditionally, unlike the Insights equivalent in buildLifecycleListeners: reporting ships enabled, so someone who trimmed Jackson never opted out of it and would otherwise have no signal that it is off. The GraalVM and integration manuals, and the upgrade guide, now say so. Result: Excluding Jackson costs the report and nothing else; SESSION_ID is unaffected. The absent-classpath path cannot be exercised in-process, so the test pins the part that can be: that the substitute contributes nothing and does not need a context to say so. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Force-pushed No new SPI methods. Schema resynced to v6 ( On the five findings in #974's review — checked each against 4.x, and only one transfers, already implemented:
|
|
CI on Update: re-run green. The lane passed on attempt 2 in 4m57s, against 8m28s on the attempt that failed — same commit, so the runner, not the code. Whole run is green now. |
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 context's policies, serialized to the normative cross-driver JSON
schema shape. Stage 1 (#967) is merged; this branch is rebased onto
scylla-4.x, so there is nostage-1 noise in the diff.
Gated behind
advanced.driver-config-reporting.enabled, which ships enabled (per@dkropachev's cross-driver review). Turning it off suppresses only the
DRIVER_CONFIGblob —SESSION_IDrides on every connection unconditionally, independently of this flag, so "off" is not"zero change on the wire".
Read the commits in order; each is formatter-clean and green on its own.
The report 🧩
Built from the default execution profile + the context's policies, and rebuilt on every
control-connection init, so it always reflects the current (possibly runtime-reloaded) config.
Three groups:
connection(connect timeout, request capacity, pooling, socket options,reconnection policy, TLS when on, and the datacenter preference that scopes pooling),
control-plane(internal-query and schema-agreement timeouts), andquery(per-requestdefaults, plus the retry, load-balancing and speculative-execution policies).
Against the shipped default configuration, 938 bytes (pretty):
{ "version": 1, "connection": { "connect": { "timeout-ms": 5000 }, "requests": { "in-flight": { "max": 1024 }, "orphaned": { "max": 256 } }, "pool": { "shard-aware": { "enabled": true } }, "socket": { "tcp-no-delay": true, "keep-alive": false, "reuse-address": false }, "reconnection": { "policy": { "type": "exponential", "base-ms": 1000, "max-ms": 60000 } }, "node-preference": { "type": "dc-auto" } }, "control-plane": { "queries": { "system": { "timeout": { "client-side-ms": 5000 } } }, "schema": { "agreement": { "timeout-ms": 10000 } } }, "query": { "defaults": { "page": { "size": 5000 }, "consistency": "LOCAL_ONE", "serial-consistency": "SERIAL", "idempotence": false, "client-timestamps": true, "request": { "timeout-ms": 2000 } }, "retry": { "policy": { "type": "standard-error-aware" } }, "load-balancing": { "policy": { "type": "token-aware", "load-distribution": "shuffle", "fallback-to-non-preferred-nodes": false, "adaptive-ordering": { "signals": [ "response-rate", "in-flight-requests", "recovery-state" ] } }, "node-preference": { "type": "dc-auto" } } } }Verified on the wire by a
tsharkcapture of theSTARTUPframes against a single-node CCMScyllaDB (protocol v4, default config):
SESSION_IDon every connection,DRIVER_CONFIGonly on thecontrol connection, and gone when the flag is off. Verified end to end through
system.clients.client_options(ScyllaDB 2026.1) andsystem_views.clients(Cassandra 4.1),untruncated, with the one backend-conditional key differing as it should.
Invariants 🔒
SESSION_IDis still emitted and only the config blob is dropped.
RuntimeExceptiononly — deliberately notbare
Error, soOutOfMemoryError/StackOverflowErrorstill surface. The oneErrorthis classcan provoke is the
InternalErrorthatgetClass().getSimpleName()raises for certain syntheticclasses; that is caught at the call site, behind a package-private seam so the branch stays
testable.
manual/core/integrationdocuments that the driver "can operatenormally without" Jackson, and
DefaultDriverContextalready honours that for Insights by checkingDefaultDependencyChecker.isPresent(JACKSON)before building the listener. Reporting was a thirdJackson user with no such check, and it is default-on and on the connection-init path — so on that
classpath linking
DefaultDriverConfigReporterraisedNoClassDefFoundError, anErrorraisedwhile resolving the class rather than from any method it declares. Neither the
try/catchabovenor
ProtocolInitHandlercould contain it: every control connection failed and the session couldnot be built. Now the implementation is chosen up front, falling back to a
NoopDriverConfigReporterthat names no Jackson type anywhere (one reference would make loading it fail for exactly the
deployments it exists to serve). Logged unconditionally, unlike Insights: nobody opted in to
reporting, so nobody would think to look for a message saying it is off. Verified both ways against
core's real runtime classpath with its Jackson jars removed.
DefaultSession.init()already forces eagerly. It was the one component on the reporting path left lazy, which made a
Netty event loop the first thread to load it and Jackson — jar reads, mid-
STARTUP. This is alsowhat makes the ordering
buildJson()'s javadoc relies on true by construction.STARTUPoptionvalues go through
ByteBufPrimitiveCodec.writeString, which writes a 16-bit length prefix viaByteBuf.writeShortwith no bounds check, so a value over 65535 bytes silently truncates theprefix modulo 65536 while still appending the whole body — a corrupt frame and a failed handshake,
and not something the
try/catchcan save, since nothing throws. Parts of the report areuser-supplied and unbounded (DC/rack names, consistency levels, custom policy class names), so
without this the "reporting must never prevent a connection" invariant simply wasn't true. Measured
on the UTF-8 bytes, since that is what the prefix counts.
as
null. Same where an optional key's configured value is outside what the schema can express(a disabled request timeout, a disabled
SO_LINGER, an unbounded page size), and same where theanswer is genuinely unknown — the two cases the schema made optional for exactly that purpose.
no-fallback getters, which throw on an absent option, so a config source omitting any one of them
dropped all ~34 fields behind a single WARN. Every read now either sits behind
isDefinedorpasses an explicit fallback, and the schema picks which: an optional field falls back to the same
"disabled" sentinel that already omits it, a required one to the value
reference.confdocuments.Design decisions worth questioning 📐
instanceof— so a user subclass of abuilt-in falls through to
{type:"custom", name:<class>}instead of being misreported as theunmodified built-in.
connection.reconnection.policyand
query.speculative-execution.policydescribe the policy that is actually reconnecting andspeculating. The built-ins latch these numbers into final fields when the context builds them, and
advanced.speculative-execution-policyis documented as not modifiable at runtime, so areloaded profile can carry values no request executes with — and, unlike a constructor, admits
values the schema rejects: a negative
delay-ms, or amax-executionsof 1 that would drop thewhole group while the policy still speculates. Reading the instance makes those ranges hold by
construction.
adaptive-orderingandfallback-to-non-preferred-nodeswere the last two readingthe profile — the class javadoc used to name them as the exception — and now read
DefaultLoadBalancingPolicy.isAvoidingSlowReplicas()andBasicLoadBalancingPolicy.getMaxNodesPerRemoteDc()instead, so no latched value is described froma profile the running policy has not adopted. Raised by @dkropachev for the first; the second is
the same defect one field over, fixed alongside it.
control-plane.queries.system.timeout.server-side-msreports configuration, not effect.CassandraSchemaQueriesadds aUSING TIMEOUTclause built fromadvanced.metadata.schema.request-timeoutonly where
shouldApplyUsingTimeout()sees sharding info, so on generic Cassandra the option is aclient-side wait alone. This was gated on that signal until @dkropachev asked for the configured
value regardless of peer detection — the way
pool.shard-aware.enabledalready reports intent. Thecost is that an operator on Cassandra 4.1 reads a server-side timeout nothing enforces, which wants
the schema description to say so; the gain is that the report no longer depends on anything the
peer said, which removed the
NodeShardingInfoargument entirely.getSslHandlerFactory(), notgetSslEngineFactory()(per@sylwiaszunejko). The handler factory is the reference
ChannelFactoryinstalls the SSL handlerfrom, and
buildSslHandlerFactory()is the documented expert extension point (e.g. Netty's nativeOpenSSL): an override supplies no engine factory, so reading the engine factory reported such a
session as plaintext when it is in fact encrypted. Host name validation is then read off the engine
factory the active handler actually wraps — never through the context, which can name a different,
unused one and whose
LazyReferencethe reporter would be the first to force (keystore reads on aNetty event loop, mid-
STARTUP).SslEngineFactory.isHostnameValidationRequired()andTimestampGenerator.isClientSide()returnOptional<Boolean>, empty by default. Host namevalidation is a property of the JDK
SSLEngine, unreadable through an opaque handler factory; anda custom
TimestampGeneratoris free to returnStatement.NO_DEFAULT_TIMESTAMPand delegate tothe coordinator, which no class check can detect and which calling
next()to find out would haveside effects. Both keys are now optional in the schema with absence defined as unknown, so an
implementation that cannot answer is reported by omission rather than by a guessed boolean — which
for these two fields would misdescribe a security control and a write-timestamp source. Both
methods are
default, so existing implementations keep compiling. Not a Java-local flourish:@dkropachev asked Bump ch.qos.logback:logback-classic from 1.2.3 to 1.2.13 #263 for exactly this shape on both fields — emit the boolean only where it
is known, omit it for custom or unknown behaviour — and cited this PR's timestamp accessor by
name as the model. (He also asked Bump ch.qos.logback:logback-classic from 1.2.3 to 1.2.13 #263 to derive
client-timestampsfrom the negotiatedprotocol, since
SupportsTimestamp()starts at v3; Java 4.x supports nothing below v3, so thereis nothing to gate on here.) One thing the javadocs now spell out: a subclass of a built-in
inherits its parent's answer rather than the empty default, so a subclass that changes what
these describe has to override them too.
holds these options as
Durationand schedules several in nanoseconds, so truncating a 500 µstimeout to
0would report a live timeout as the very value the field defines as off. Applies toschema.agreement.timeout-ms,queries.system.timeout.client-side-ms,query.defaults.request.timeout-msandreconnection.policy.delay-ms. Three fields aredeliberately exempt, because
0is what they really mean there:connection.connect.timeout-ms(Netty's
CONNECT_TIMEOUT_MILLIStruncates identically, and 0 disables it),...server-side-ms(the value goes on the wire as a
USING TIMEOUTmillisecond argument, so sub-millisecond reallyis
0msserver-side) andspeculative-execution.policy.delay-ms(reference.confdocumentssub-millisecond delays as equivalent to 0).
connection.requests.orphaned.maxis the effective threshold, not the configured one.ChannelFactoryrequiresmax-orphan-requeststo stay belowmax-requests-per-connectionandsilently substitutes a quarter of the latter otherwise. Reporting the configured value would
describe a threshold no connection was built with, so the correction lives in one place —
ChannelFactory.effectiveMaxOrphanRequests(), which the channel setup itself calls.node-preferenceslots are filled differently, because in Java they mean differentthings.
computeNodeDistancederives node distance from the local DC alone — a node outside it isIGNORED, and anIGNOREDnode gets no pool — so the datacenter genuinely scopes which nodes areconnected to, and goes under
connection.node-preference. The rack never reaches that method: itonly reorders replicas at the head of a query plan, with connections still held across the whole
local DC. So the full preference (rack included) goes under
query.load-balancing.node-preference,and the connection group carries the datacenter half alone. Emitting the same object in both would
claim a rack-scoped connection pool that does not exist.
load-distributionisshuffleandadaptive-orderingmaps to slow-replica avoidance. Thebuilt-ins shuffle the replica head of every query plan unconditionally
(
BasicLoadBalancingPolicy.shuffleHead, no config to disable), soround-robinwould describe onlythe non-replica tail and
replica-setwould claim the order is untouched (see A1 for the one casethis misses). Java has no latency-percentile ordering, so
adaptive-orderingmaps to the one realmechanism,
DefaultLoadBalancingPolicy's slow-replica avoidance, with its signals read offavoidSlowReplicasrather than guessed — andlatencydeliberately absent, since those samplesrecord when responses arrived, not how long they took. Its presence is also now the only thing
distinguishing
BasicLoadBalancingPolicyin the report, which has no such mechanism at all.Spec conformance 🔍
The v1 schema is shipped verbatim as a test resource, byte-identical to the design document's
schema block, and every representative report is validated against it in
DefaultDriverConfigReporterTest— enforced, not asserted. A negative test confirms the validatoractually rejects an out-of-schema document.
Every report a stock configuration can produce validates. Two required fields are constrained more
tightly than the option behind them, but only one is reachable through a running driver. Both are
reported truthfully and pinned by tests that assert the violation:
query.defaults.consistencyis a closed enum whilebasic.request.consistencyis an unvalidatedstring. The built-in load balancing policies resolve it through the
ConsistencyLevelRegistryintheir constructor, so an unknown name fails the session before any report exists — reaching this
needs a custom registry defining extra names, which is the case CodeRabbit raised. This is the
one real gap.
connection.requests.in-flight.maxmust be positive, and nothing validatesadvanced.connection.max-requests-per-connectionagainst that —ChannelFactoryhands the valuestraight to
StreamIdGenerator, which does not range-check it. An earlier revision of thisdescription claimed such a setting starts a session; it does not. The connection fails first: a
negative value makes
StreamIdGenerator'sBitSetthrow whileChannelFactoryis still buildingthe channel, and
0leaves no stream id for the control connection's ownOPTIONS, whichChannelHandlerRequestfails onpreAcquireIdbeforeSTARTUPis composed. So this is unreachableby construction, not a live exposure. The value is still passed through and still pinned, so the
behaviour stays defined if the driver ever stops failing that early. (The same setting would also
drive
orphaned.maxnegative — a second reason to read it as one unreachable shape rather than onefield's gap.) Worth one cross-driver note, since Bump ch.qos.logback:logback-classic from 1.2.3 to 1.2.13 #263 was asked to change this very field:
there the reported number was the pool-admission threshold rather than the stream-id pool, and
@dkropachev asked for
Connection.GetMaxConcurrentRequests(128 or 2048) instead. In Java thetwo are one number —
ChannelFactoryisnew StreamIdGenerator(maxRequestsPerConnection)—so the configured value already is the stream-id pool size and needs no such correction.
A third shape was reachable until the push before last:
query.speculative-execution.policytook both its numbers from the profile, so a reload could put a negative
delay-ms— whichnonNegativeIntegerrejects — into an otherwise valid document, or drop the group while the policystill speculated. Both now come off the policy, whose constructor admits neither.
Fabricating an admissible value would misreport a setting an operator may have chosen deliberately,
and dropping the whole report would punish every other group for one field.
Approximations, flagged not changed⚠️
load-balancing.policy.load-distributionshufflenewQueryPlanPreserveReplicas, which never shuffles —replica-setin schema terms — anddefault-lwt-request-routing-methodships asPRESERVE_REPLICA_ORDER. So every LWT statement on a default config is distributed the way the report says it is not. No single enum value is honest.load-balancing.policy.fallback-to-non-preferred-nodesmax-nodes-per-remote-dc > 0and a datacenter preference existsRoundRobinPolicy, "for rr, there is no remote nodes or nodes outside of the node preferences, so having ittruewill be confusing, and yes, having it asfalsewill be less confusing, not having it at all would be better, but there is no good way to do that" (the tail of that is now a schema follow-up). One term is still missing.maybeAddDcFailoveralso consultsisDcFailoverAllowedForRequest, false for a DC-local consistency whileallow-for-local-consistency-levelsis off — and both of those ship as the default, so on a config that changes nothing butmax-nodes-per-remote-dcthe report saystruewhile no ordinary statement fails over. Note the "it's per-request, a statement can override it" argument does not carry on its own:query.defaults.consistencyis published under the same caveat. The real cost is that closing it needsConsistencyLevelRegistryresolution of a string this report deliberately passes through unvalidated. A schema value meaning "conditional" is the honest fix.connection.socket.keep-alive,.reuse-addressfalsewhen unsetStandardSocketOptionsdocuments as system dependent.falseholds for JDK NIO on Linux; unverified for the native transports. Both keys are required, so omission is not available. Narrower than it looks beside #263, where @dkropachev found csharp'sReuseAddresswas never wired toSO_REUSEADDRat all:DefaultNettyOptionsdoes set bothChannelOptions whenever the option is defined, so only the unset case is approximated here.control-plane.queries.system.timeout.client-side-msCONTROL_CONNECTION_TIMEOUTMETADATA_SCHEMA_REQUEST_TIMEOUT, so the two siblings do not describe the same query — an operator debugging a slow schema query reads the wrong number. Already on the thread with @dkropachev; the fix is aqueries.schemasibling, blocked today byadditionalProperties:false.node-preferencedatacenter / rack values""is reported as no preference whileOptionalLocalDcHelper/OptionalLocalRackHelperhand it to the policy as a set-but-unmatchable datacenter. No alternative:nonEmptyStringleaves no way to report"", andtype:"dc"with the key omitted is invalid too. A padded value is no longer normalized —nonEmptyStringisminLength: 1, so" dc1 "is valid to emit and trimming it hid the typo an operator opens this report to find (raised by @dkropachev). Normalizing the runtime helpers instead was declined: that changes routing, in a reporting PR.query.load-balancing.node-preferencetype:"rack"DefaultLoadBalancingPolicy;BasicLoadBalancingPolicynever readslocalRack, andPRESERVE_REPLICA_ORDERignores it as well. Kept — the value is configured, and hiding a real setting is the worse failure mode.node-preferenceslots, when a node-distance evaluator is configuredbasic.load-balancing-policy.evaluator.classis consulted bycomputeNodeDistancebefore the datacenter and its verdict returned directly, so it can leave an in-DC nodeIGNOREDand without a pool. Nothing can be reported for it: the option names a user-supplied class, the driver ships no location-based evaluator to introspect, andnode-location-preferencehas no slot for a class name. Raised by @dkropachev on the gocql sibling, whereDataCenterHostFilteris introspectable.node-preferenceslots, for a custom load balancing policyconnection's says the DC decides which nodes hold a pool, which holds becauseBasicLoadBalancingPolicy#computeNodeDistancemakes an out-of-DC nodeIGNORED;query.load-balancing's says it scopes routing. A custom policy computes distance itself and need not readlocal-datacenterorwithLocalDatacenterat all. Kept on A6's grounds. Deliberately asymmetric with the no-DC case, where the group is omitted rather than reporting adc-autothe SPI never promises: nothing is inferred on a custom policy's behalf, while what was configured is passed through.Two cosmetic ones, noted for completeness: a negative
schema.agreement.timeout-msnormalizes to0(same outcome as
0, one extra round trip, and the schema cannot say "negative"); andconnection.connect.timeout-msis reported as a fulllongwhileDefaultNettyOptionsnarrows itwith
intValue(), so a connect timeout past ~24.8 days wraps in Netty.Follow-up ⏭️
For the schema owner — all for the document rather than here.
core/src/test/resources/config/driver-config-report-v1.schema.jsonis a byte-for-byte copy of the document's normative block, so every item below lands there first
and the vendored copy is resynced afterwards. Adding a key here to close a review comment would fork
the contract and leave this driver's conformance suite validating against a schema no other
implementation has.
describe fields the schema no longer has, and both sample payloads still show the pre-restructure
flat envelope, so they fail validation against the document's own schema.
$idandversionstill say v1 /const: 1although earlier revisions removed a requiredtop-level group and renamed load-balancing fields. By the schema's own versioning rule that is a
major bump; harmless while every implementation is unreleased, but a v1 consumer cannot tell the
shapes apart.
dc-autocarries the inferred value in plainlocal-dcwhilerack-autouses an explicitinferred-prefix. Implemented as specified; the asymmetry is easy to misread.node-location-preferencehas no "no preference" variant (raised by @dkropachev). Omitting theoptional group is the schema-valid answer and is what this PR does, but a
nonetype would say itpositively.
query.defaults.consistencyneeds either a wider type or a documented rule for names outside itsenum — the one conformance gap a running Java driver can still produce. (An earlier revision of
this list also asked the spec to define consumer behaviour for a non-positive
in-flight.max;withdrawn — see Spec conformance, no session can reach it.)
control-plane.queries.system.timeoutgroupsclient-side-msandserver-side-msas two views ofone timeout. For Java they are not — see A4; a
queries.schemasibling would let each class ofquery carry an honest pair. Agreed on the thread, and note the two asks interact: once
queries.schemaexists,METADATA_SCHEMA_REQUEST_TIMEOUTbelongs there rather than underqueries.system, so theserver-side-msthis branch ungated will migrate.server-side-msneeds the "reports configuration intent" clausepool.shard-aware.enabledalreadycarries, now that it is emitted on backends where no
USING TIMEOUTclause is ever sent.connection.poolshould carrylocal.sizeandremote.size(requested by @dkropachev; bothoptions are always configured and consumed by
ChannelPool). Blocked here:$defs/connection-poolis
additionalProperties: falseand this branch ships the schema block verbatim. Two things tosettle in the shape — whether a size of
0is representable, sincepositiveIntegerwouldreproduce the objection raised against the old
desired-connections-count; and thatChannelPool.initialize()ceil-divides the configured size across shards, solocal.size = 1on a4-shard node opens four connections and the number Java reports is not the connection count.
speculative-execution.policy.percentileisexclusiveMinimum: 0, while 3.x'sPercentileSpeculativeExecutionPolicyaccepts0.0— so an accurate report of that configurationis out of schema (raised by @dkropachev on Client config reporting (3.x) — stage 2: full DRIVER_CONFIG report #974). Unreachable from Java 4.x, which has no percentile
policy at all, but the schema is shared.
fallback-to-non-preferred-nodesshould be optional, so that "there is no node preference,therefore no non-preferred nodes to leave" can be said by omission rather than by a
falsethatreads like a disabled feature. This is the tail of @dkropachev's Bump ch.qos.logback:logback-classic from 1.2.3 to 1.2.13 #263 comment quoted in A2 —
"not having it at all would be better, but there is no good way to do that" — and it retires
half of A2.
standard-error-awarehas no normative rule set: its whole description is "Standarderror-aware retry policy." @dkropachev challenged the csharp mapping on rules the spec does not
state (csharp's
DefaultRetryPolicynever retriesUnavailable). Java's does, so thatobjection does not transfer — but no implementation's mapping is checkable until the type says
what it means.
Other:
Client config reporting (3.x) — stage 2: full DRIVER_CONFIG report #974's and Bump ch.qos.logback:logback-classic from 1.2.3 to 1.2.13 #263's (39128 bytes each); stage 2: populate the DRIVER_CONFIG report gocql#987's differs in exactly two places —
$defs/requests/requireddropsorphaned, andorphaned.max's description gains "Absent onlywhen this bound is unknown, for example when the client never replaces a connection over
accumulated orphans and so has no limit to report." So either the document moved past the
revision these three track, or that copy was edited locally and gocql's conformance suite
validates against a forked contract. Asked on that thread, unanswered. Deliberately not
resolved here: the resource has to stay a byte-for-byte copy of the document's normative
block, and either way the change is permissive — Java always has an orphan limit, so nothing
this branch emits changes.
restructure. The sub-millisecond reconnection floor does not carry over: 3.x's
ConstantReconnectionPolicyholds along delayMs, so there is no sub-millisecond value totruncate. Separate PR, separate branch. Traffic goes the other way too: the
speculative-execution source-of-truth fix on this branch was raised there first, and 3.x additionally
had to stop reporting both built-ins as
custom, which this branch never did.default-on flip is intentional; reasoning is on the threads.
DriverBlockHoundIntegrationITis JDK 14+ only and was not run locally. With reporting on bydefault the report is built on a Netty event loop; reasoned safe (no SSL factory resolution or IO
with the default config, and Jackson is in-memory), but worth watching in CI. The larger half of
that risk is gone: the reporter is now resolved during session init, so the event loop is no longer
the first thread to load it and Jackson.
here.
manual/render with a spurious#prefix (href="#../configuration/reference/") — a site-wideMyST artifact affecting pre-existing links too, so it wants its own issue rather than a partial fix
here. The one link this PR would have added was dropped for that reason.
🤖 Generated with Claude Code