Drop obscure features - #2
Conversation
6122754 to
2aae00e
Compare
nikagra
left a comment
There was a problem hiding this comment.
Reviewed the three commits stacked on #1 (OSGi, core-shaded, GraalVM native-image). The removals are thorough where it counts: no build references left to core-shaded, osgi-tests, maven-bundle-plugin, felix/pax-*/graalapi or <packaging>bundle</packaging>; distribution*, bom and binary-tarball.xml are consistent; the deleted manual/* toctree entries are complete; and the three commits are individually bisect-safe (the OSGi commit fixes core-shaded/pom.xml before the later commit deletes it).
Two things I checked because they looked risky and turned out fine, so you don't have to re-tread them:
- JPMS module names survive the
bundletojarswitch.Automatic-Module-Nameis set bymaven-jar-plugin's<manifestEntries>(core/pom.xml:217), not by bnd's<instructions>, socom.datastax.oss.driver.coreis unchanged for module-path users. - No new formatting failures. None of the four modified Java files has an import orphaned by the deletions, so this PR adds nothing to the
fmt:checkbacklog from #1.
Commenting rather than requesting changes — nothing here breaks the build. The two substantive notes are the same shape: the upgrade guide declares a removal complete while part of it is still shipped or still documented the old way.
Three findings have no line in the diff to hang off:
.snykis now dead config. All three ignores aregraal-sdkCVEs, and bothorg.graalvm.sdk:graal-sdkandorg.graalvm.nativeimage:svmdeclarations are gone. It is also referenced only in the workflow'spaths-ignore, so nothing in CI reads it.DefaultDriverConfigReporterTest.java:574justifies its anonymous subclass with "a real subclass of it already exists elsewhere in this repo's test code (osgi-tests'CustomRetryPolicy)" — deleted by this PR.- The description records no verification. For a change that flips five modules from
bundletojar, discontinues a published artifact and drops a reactor module, one recordedmvn verifyis what would settle theguava-shadedquestion below.
Each comment is a one-line TL;DR with the detail folded underneath.
59b87ec to
cfd4333
Compare
nikagra
left a comment
There was a problem hiding this comment.
Re-reviewed at cfd43331b0. All seven round-1 items are addressed — the META-INF/native-image/** configuration, .snyk, the six OSGi-rationale rewrites, the three ReflectionTest cases, clean-classes, the reporter-test comment and the 4.13.0 link. Not approving yet: one Major and three Minors below, all anchored in the diff.
A correction of mine. Round 1 told you to drop clean-classes because its stated rationale was dead. That was right about the rationale and incomplete about the effect — the execution also made target/classes hermetic, and that property went with it. Details in that thread; not a blocker.
Verified clean, so you can skip re-checking these. No surviving osgi/felix/bnd/graal/core-shaded references outside the design docs and the upstream-only changelog. Automatic-Module-Name genuinely survives bundle→jar — I checked real manifests in ~/.m2: core, query-builder, mapper-runtime and test-infra carried it, metrics and guava-shaded never did. GraalDependencyChecker had no remaining callers. distribution*, bom, binary-tarball.xml, the Makefile and the workflows carry no dead references. The deleted OsgiLz4IT/OsgiSnappyIT/OsgiReactiveIT were @Ignored and have live replacements in integration-tests.
cfd4333 to
461819e
Compare
nikagra
left a comment
There was a problem hiding this comment.
Re-reviewed at 461819e5. All seven round-2 items check out — the META-INF/native-image/** config is gone tree-wide, .snyk and its paths-ignore readers are gone, the OSGi-derived rationales are rewritten, ReflectionTest is at 7 cases including the two-loader-exhaustion and 3-arg overload paths, the assembly META-INF/MANIFEST.MF exclude is in, and clean-classes is restored. No dangling references to any deleted class, module, doc page or managed dependency; no module still declares packaging>bundle; and core keeps its Automatic-Module-Name via maven-jar-plugin. Two Minors and two Nits inline, none blocking.
One more with no line to hang it on: [Minor] 🟡 core/src/main/java/com/datastax/oss/driver/internal/core/util/Dependency.java:32-34 still tells readers these libraries "may be shaded" and that the shade plugin rewrites the literals to com.datastax.oss.driver.shaded.*. That was true of exactly one artifact — java-driver-core-shaded, which relocated Jackson — and this PR deletes it. guava-shaded relocates com.google only, and none of the five entries is a com.google class, so the paragraph now describes a contract no build step implements. Same cleanup as the sibling DefaultDependencyChecker javadoc this PR already fixed.
OSGi is unsupported in the java-rs-driver (plan D6): the ecosystem is niche and loading a native library under OSGi classloaders is a problem of its own. Delete the osgi-tests module and the OSGi manual page, and stop producing bundle manifests: the maven-bundle-plugin is removed from every module (packaging goes back to jar), together with the OSGi/Felix/ Pax-Exam dependency and property declarations. The shaded modules generated their jar manifest with that plugin, so their assembly executions now let maven-assembly generate a default one. The ClassLoader-taking public API is untouched. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The shaded core artifact exists to relocate Netty and Jackson out of the way of the application's own copies. The Rust core takes over networking, so Netty is being removed from the driver and the artifact loses its reason to exist. Delete the module, its manual page, and the references from the reactor, the BOM and the manual. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
GraalVM native images are unsupported in the java-rs-driver (plan D6). Delete the substitution classes (compressors, metrics factory, libc, request processors, Guava's Unsafe comparators in the shaded artifact), the Graal-specific libc implementation and dependency checker, the org.graalvm build dependencies, the GraalVM manual page and the META-INF/native-image configuration. The configuration has to go with the classes: native-image auto-applies the Args= line of every native-image.properties on the classpath, so leaving it behind would keep configuring downstream native-image builds while the substitutions that kept optional dependencies out of the closed-world analysis no longer exist. The .snyk policy goes too - all three of its ignores were graal-sdk CVEs. Native.LibcLoader now always goes through JNR (falling back to the empty implementation as before), which is what every non-native-image deployment already did. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ClockSeqAndNodeContainer set the volatile `initialized` flag before assigning `val`, the opposite order from the memoized supplier it was adapted from. A second thread arriving in that window sees `initialized == true`, skips the lock and returns `val == 0` while the first thread is still inside makeClockSeqAndNode() - which hashes every local address and looks up the PID, so the window is tens of milliseconds on first use. Every time-based UUID minted in it carries clockSeqAndNode = 0: wrong variant bits, and a node part shared with every other host in the same state. If makeClockSeqAndNode() throws, the container stays poisoned at 0 for the lifetime of the JVM. Swap the two statements so the value is assigned first and the volatile write that publishes it happens last. Reported in review of the GraalVM removal, where the comment above this class was being rewritten. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
461819e to
79d3b1b
Compare
|
Round 3 addressed at Right, and removed at 64b68e3 (the
So the contract had no implementer left, exactly as with the sibling |
This PR drops obscure, legacy public features of the driver:
maven-bundle-pluginand its<instructions>are gone from all five modules (<packaging>bundle</packaging>→jar), and theosgi-testsmodule, the Felix/Pax-Exam dependencies and the OSGi manual page are deleted.Automatic-Module-Nameis unaffected — it is set bymaven-jar-plugin, so JPMS module names are unchanged for module-path users.core-shadedmodule — its purpose was relocating Netty and Jackson away from the application's copies; with the networking layer moving to the Rust core, Netty is on its way out of the driver entirely.org.graalvmdependencies, the manual page, and theMETA-INF/native-image/**configuration, which would otherwise have kept injectingArgs=into downstream native-image builds with nothing left to substitute out. The now-dead.snykpolicy (threegraal-sdkCVE ignores) goes with it.I hope no one will miss them.
Review round 3
Uuids: the lazy-init comment now states JAVA-2663 as the historical reason instead of describing a constraint that no longer exists (three of the four PID sources need no native infrastructure). The holder stays — the laziness is what JAVA-2663 asked for. A separate commit fixes a pre-existing race the reviewer spotted next to it:initialized = truewas written beforeval = makeClockSeqAndNode(), so a second thread could mint time-based UUIDs withclockSeqAndNode = 0(wrong variant bits, node part shared across hosts) for the tens of milliseconds the first thread spends hashing local addresses — and forever if that call threw.META-INF/MANIFEST.MFexclude moved into theDrop OSGi supportcommit, alongside the<archive><manifestFile>removal it belongs with, so no commit leavesguava-shadedwith two unpinned manifest contributors. All three commits' assembly descriptors now parse as XML — the first attempt at moving it left the file without its closing</excludes>at one commit, which would not have built.ReflectionTest: the thrice-duplicated always-throwing loader is oneblindLoader()helper, plus a new case for theLinkageErrorarm ofcatch (LinkageError | Exception e)that nothing covered. 8 cases.Native:LibcLoadercollapses toprivate static Libc loadLibc()— no class left implying a choice of implementation.Dependency's javadoc no longer claims the shade plugin rewrites these package names:core-shaded(which relocated Jackson) is deleted here, andguava-shadedrelocates onlycom.google, which none of the five entries is.Review round 2
SessionBuilder.withClassLoader's javadoc no longer promises that a partially-resolving loader is safe in general: reflective class loading retries with the driver's loader, configuration resources do not, so anapplication.confthe given loader cannot see is silently ignored. The paragraph states that asymmetry instead of narrating what the javadoc used to say.guava-shaded:clean-classesis restored (with the rationale that actually applies —dependency:unpackoverwrites but never deletes, so a stale entry from a previous Guava version would otherwise reach the published jar on a non-cleanbuild), and the assembly fileSet now excludesMETA-INF/MANIFEST.MFso maven-archiver is the manifest's single contributor, as the OSGi plugin used to guarantee.java-driver-core-shaded(Netty/Jackson version clashes with the application's own copies, Jackson especially, since config reporting detects it reflectively), and the GraalVM list now names the request-processor substitutions and the Graal dependency checker.ReflectionTestgrows to 7 cases: the two-loader exhaustion path and the default-packages overload — the branches the deletedOsgiCustomLoadBalancingPolicyITused to cover — are now pinned, not just the single-loader ones.Review round 1
All four inline comments and the three anchorless findings are addressed:
SessionBuilder.withClassLoader,DriverConfigLoader,DefaultDriverConfigLoader,DefaultProgrammaticDriverConfigLoaderBuilder,ReflectionandDefaultDependencyCheckernow state why the driver-loader fallback stays (layered class loaders in web apps and application servers), instead of citingDynamic-Import:*or Apache Felix;ReflectionTestcases replace the custom-ClassLoadercoverage that died with the OSGi tests: the two-step fallback, user-loader-first ordering, and the null return;guava-shaded's deadclean-classesexecution is dropped, and the sources-jar question is answered below;osgi-tests' CustomRetryPolicyreference inDefaultDriverConfigReporterTestis gone, and the historical 4.13.0 upgrade-guide entry no longer links the deleted GraalVM manual page.Verification
JDK 17, on the rebased branch:
mvn clean verify -DskipTests— BUILD SUCCESS across the reactor (this ismake check:fmt:check,revapi:check, jar manifests, javadoc).mvn clean verify -pl guava-shaded— still attachesjava-driver-guava-shaded-…-sources.jar(6.9 MB, 668 entries, 655 of them relocated undercom/datastax/oss/driver/shaded/guava), so a release would not be rejected for a missing sources jar.maven-source-plugincontributes nothing now that the module has no Java sources; shade'screateSourcesJarbuilds it from the dependencies' sources. The final jar is 2038 entries, 2020 of them undercom/datastax/oss/driver/shaded/guava, zero unshadedcom/google/, and its manifest is maven-archiver's (Manifest-Version: 1.0).mvn test— green except the 20 knownTimestamp*CodecTest.should_parsecases, which are parameterized overZoneId.systemDefault()and so fail on any non-UTC machine and pass in CI.coregoes from 3130 to 3137 tests with the newReflectionTestcases.find . -path "*META-INF/native-image*"— empty.