Conversation
nikagra
left a comment
There was a problem hiding this comment.
Reviewed rs..drop-dse (4 commits, 573 files). The DSE removal itself is thorough: no dangling com.datastax.dse code references, the OptionsMap/reference.conf default counts still balance on both sides so MapBasedDriverConfigLoaderTest.should_fill_default_profile_like_reference_file still passes, and cross-checking the equivalent upstream change (scylladb#1042), the inverted DefaultCcmBridgeBuilderCustomizer condition and the renamed serialized Version.dsePatch field are both handled correctly here.
Requesting changes on three build gates. None have ever run: the workflow only triggers on PRs targeting scylla-4.*x, so no rs-based branch has had CI. I verified the formatting one by running the formatter rather than reading it — 49 of the 136 changed files fail, 0 on rs.
Two behaviour regressions are inline as well: the GraalVM reactive substitutions were deleted along with the graph ones, and plaintext auth lost the SASL mechanism negotiation that HCD — which this PR keeps — still needs.
Each comment is a one-line TL;DR with the detail folded underneath. Several of these are Java build machinery with no Cargo equivalent, so the folded part explains the mechanism rather than just naming the plugin.
The description records no verification; one make check run would have caught all three gates before review.
Java-RS-Driver-design.md is the initial assessment of what it takes to replace the driver internals with the Rust driver. Java-RS-Driver-plan.md turns it into a task-focused plan with the open questions decided. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The reactive execution API (executeReactive, ReactiveResultSet, ReactiveRow, MappedReactiveResultSet and friends) lives under com.datastax.dse for historical reasons only: it landed there when the DSE and OSS drivers were unified. Nothing about it is DSE-specific, and it must survive the removal of DSE support, so relocate it: dse.driver.api.core.cql.reactive -> oss.driver.api.core.cql.reactive dse.driver.internal.core.cql.reactive -> oss.driver.internal.core.cql.reactive dse.driver.api.mapper.reactive -> oss.driver.api.mapper.reactive dse.driver.internal.mapper.reactive -> oss.driver.internal.mapper.reactive Two shared helpers used by non-DSE code move along with it: BoundedConcurrentQueue (to oss.driver.internal.core.util.concurrent) and AuthUtils (to oss.driver.internal.core.auth). This is a pure relocation: no behaviour or content change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
nikagra
left a comment
There was a problem hiding this comment.
Re-reviewed at 6e06019e73. All nine round-1 items are addressed, and the three build gates now pass rather than being unexercised. Approving. One Minor and two Nits below, none blocking.
The gates now run. Round 1 said none of them ever had — that was true when written and is obsolete now. 142cb944f on rs retargeted the workflow trigger, so this PR gets CI, and it is green 22/22: Build (11)/(17) (Error Prone 2.31.0 loads on JDK 11), Full verify (11)/(17) (= make check = mvn verify -DskipTests, which is where fmt:check and revapi:check live), plus the full Cassandra 3/4-LATEST and Scylla LATEST/LTS integration suites that round 1 flagged as never having run.
Two corrections of mine.
- I wrote "google-java-format 1.7 (the version
fmt-maven-plugin2.13 bundles)". Wrong —com.coveo:fmt-maven-plugin:2.13declares google-java-format 1.13.0. My "49 files" count was produced with the wrong formatter version. The defects I listed (import order, dangling imports, duplicates) are version-independent, were all real, and are all gone: I re-checked and 8/8 dangling imports are removed, with zero order/duplicate/unused violations across the 137 Java files this PR touches. - My
TypedDriverOptionTest.java:52item was a false positive, and I also gave the wrong path (it iscore/src/test/.../api/core/config/). Collapsed to one line that statement is 112 columns, over the 100-column limit, so the formatter correctly keeps it split. Nothing to fix there.
On the per-commit claim. CI only ever gates the tip, so I checked the middle commit separately: all 73 modified Java files at debe1ce683 are clean on import order, duplicates and unused imports. The "formatted at each commit" claim holds for the part that can be checked statically.
Your revapi question — agreed, and it is the same config upstream. scylla-4.x has the identical <versionFormat>\d+\.\d+\.\d+</versionFormat> (pom.xml:665) against ${project.groupId}:${project.artifactId}:RELEASE (pom.xml:668), and every published com.scylladb:java-driver-core is four-part (4.19.0.1, 4.18.1.0, ...) or -scylla-N — none match. With failOnUnresolvedArtifacts defaulting to false, revapi:check has been passing vacuously there too. Filed as scylladb#1054, credited to you. Flagged there that fixing the regex activates a gate that has never run, so it will surface a backlog of unbaselined breakages to triage.
Two things I did not verify, so as not to imply otherwise: no native-image build was run — the substitution is structurally correct and byte-identical to the working rs paths, but nothing here proves a native image builds without reactive-streams; and no Maven run, so every revapi conclusion is read off the config plus the Maven Central version list.
[Nit] 🟢 The description needs a refresh before merge (it becomes the merge-commit body). Lines 23 and 25 say the workflow only triggers on scylla-4.*x so CI does not run, and that ITs were not run — both are now false. The 20 should_parse failures on line 22 are also not a shared baseline: that test is parameterized over ZoneId.systemDefault(), so it fails on any non-UTC machine and passes in CI. No diff anchor, hence up here.
Also cosmetic: every reply says "New head: a3ba97a", which is one force-push stale — the live head is 6e06019e73. The content is a pure rebase so nothing is wrong, but it makes the threads harder to re-review.
The java-rs-driver targets ScyllaDB (and Cassandra) only; DSE-specific features have no Rust driver counterpart and are not going to be reimplemented. Delete them outright rather than stubbing them: - graph (TinkerPop) and geometry (Esri) support - continuous paging - DSE authentication: GSSAPI/Kerberos and proxy authentication - DSE protocol versions (DSE_V1, DSE_V2) and their frame codecs - Insights monitoring (advanced.monitor-reporting) - DSE node properties, DSE schema parsing and DSE-only metrics - DseSession/DseSessionBuilder, DseDriverOption, DseDriverConfigLoader Also drops the DSE test infrastructure (DseRequirement, BackendType.DSE, CCM DSE support) along with the DSE unit tests, integration tests and OSGi services, the TinkerPop/Esri dependencies, and every DSE section of reference.conf and the manual. Two public OSS options previously declared in DseDriverOption (basic.application.name / .version, used by SessionBuilder) move to DefaultDriverOption. Version.getDSEPatch() is kept: it is public OSS API, documented now as the legacy fourth version number. Historical records (changelog, past upgrade guide sections, revapi justifications) are left untouched; upgrade_guide/README.md documents the removal. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This PR prunes DSE-related parts of the driver. It's fully AI-authored, so requires a keen and sceptical eye.
Contents
docs: add java-rs-driver design assessment and migration plan— background documents only.refactor: move the reactive API out of the com.datastax.dse namespace— the reactive API is not DSE-specific, so it is relocated rather than deleted. No behaviour change.Remove all DataStax Enterprise (DSE) support— graph, continuous paging, geometry types, proxy/DSE auth, DSE config options and version registry entries, and the DSE test/IT suites. HCD is dropped with it: it inherits DataStax'sDseAuthenticator, so keeping it would have meant keeping DSE's SASL mechanism negotiation. Both removals are documented inupgrade_guide/README.md.Review rounds
Round 1 (nine items, all addressed): the
build: unbreak the build on JDK 25commit was dropped rather than gated, so Error Prone stays at 2.31.0 and the[11, 17]matrix and<release>11</release>are untouched;fmt:formatwas run at every commit of the stack; the GraalVM reactive substitutions were restored with the graph dimension collapsed out; HCD was dropped rather than restoring DSE's SASL negotiation; and the pinneddocs.datastax.comjavadoc links were reverted to theircom/datastax/dsepaths, since this fork's rename does not affect DataStax's published 4.17 docs.Round 2: the authorization-id wording is corrected everywhere it appears (the driver still encodes the id; Cassandra and ScyllaDB ignore it server-side), the upgrade guide gained an HCD section,
getMechanism()/MECHANISMand the now-deadCcmBridge.V4_0_0are removed, and the 42 graph-eratinkerpop/gremlinentries are gone fromcore/revapi.json.revapi, worth knowing: the gate has never compared anything in this fork — or upstream.
versionFormatis\d+\.\d+\.\d+while every publishedcom.scylladb:java-driver-corerelease is four-part, soRELEASEresolves to no baseline andcheckpasses vacuously (failOnUnresolvedArtifactsdefaults to false). Filed upstream as scylladb#1054. This PR makes the config correct —com.datastax.dseis excluded on both sides, DSE and graph-era baseline entries are deleted — but does not activate the gate, because fixing the regex surfaces a backlog of unbaselined breakages that needs its own triage.Verification
CI is green on this PR (22/22):
Build,Full verify(=make check=mvn verify -DskipTests, wherefmt:checkandrevapi:checkrun) andUnit testson JDK 11 and 17, plus the Cassandra 3/4-LATEST and Scylla LATEST/LTS integration suites.Locally on JDK 17:
mvn verify -DskipTestsgreen across the reactor;mvn testgreen except 20Timestamp/LocalTimestamp/ZonedTimestamp/TimestampMillisCodecTest.should_parsecases, which are parameterized overZoneId.systemDefault()and therefore fail on any non-UTC machine and pass in CI — not a regression and not specific to this branch.