Remove DSE Graph support and TinkerPop integration - #1036
Conversation
|
Important Review skippedToo many files! This PR contains 203 files, which is 103 over the limit of 100. To get a review, reduce the PR to 100 files or fewer by splitting it into smaller PRs or changing its base branch. Upgrade to a paid plan to raise the limit. This review couldn't start because sufficient usage credits or metered capacity aren't available. Add credits or update usage-based reviews in the billing tab, then retry. ⚙️ Run configurationConfiguration used: Organization UI Review profile: QUIET Plan: Team Run ID: 📒 Files selected for processing (203)
You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
aa660cf to
6c7b536
Compare
Remove unsupported Graph APIs, request processing, serializers, metrics, tests, fixtures, documentation, and optional dependencies. Keep DSE geometry and schema metadata for follow-up cleanup.
6c7b536 to
c9be75d
Compare
dkropachev
changed the title
There was a problem hiding this comment.
🔵 Needs a closer look
It removes a broad public API and integration surface across multiple modules, warranting final human verification despite comprehensive validation.
Pull request overview
Removes unsupported DSE Graph and TinkerPop integration while preserving generic reactive CQL, geometry, and schema metadata.
Changes:
- Deletes Graph APIs, processors, serializers, tests, fixtures, and configuration.
- Removes TinkerPop/JavaTuples dependencies and packaging hooks.
- Updates metrics, documentation, and supporting infrastructure.
File summaries
| File group | Description |
|---|---|
core/src/main/java/**/graph/** |
Removes Graph APIs, predicates, processors, results, reactive support, and serializers. |
core/src/test/java/**/graph/** |
Removes Graph unit tests and helpers. |
integration-tests/src/test/java/**/graph/** |
Removes Graph integration tests, assertions, traversal DSLs, and fixtures. |
osgi-tests/**/graph/** |
Removes Graph OSGi services, checks, and integration tests. |
test-infra/.../PatchedGremlinDslProcessor.java |
Removes the Gremlin annotation processor patch. |
test-infra/.../SessionRuleBuilder.java |
Removes Graph session options. |
test-infra/.../CqlSessionRuleBuilder.java |
Simplifies ordinary CQL session construction. |
pom.xml, core/pom.xml, core-shaded/pom.xml |
Removes managed and module-level TinkerPop dependencies. |
test-infra/pom.xml, integration-tests/pom.xml, osgi-tests/pom.xml |
Removes Graph test dependencies and annotation-processing configuration. |
distribution/pom.xml |
Removes TinkerPop and JavaTuples distribution dependencies. |
core/.../CqlSession.java |
Removes Graph session inheritance and updates documentation. |
core/.../BuiltInRequestProcessors*.java |
Removes Graph request processors and simplifies native-image substitutions. |
core/.../Dependency.java |
Removes TinkerPop runtime dependency detection. |
core/.../OptionsMap.java |
Removes Graph configuration defaults. |
core/.../MetricPaths.java, core/.../Dse*Metric.java |
Removes Graph metric definitions and parsing. |
core/.../Dropwizard*MetricUpdater.java |
Removes Dropwizard Graph metrics. |
metrics/micrometer/** |
Removes Micrometer Graph metrics and test cases. |
metrics/microprofile/** |
Removes MicroProfile Graph metrics. |
core/.../SpecificExecutionProfile.java |
Removes Graph options from Insights payloads. |
core/.../ExecutionProfilesInfoFinder.java |
Stops collecting Graph execution-profile options. |
core/.../insights/*Test.java |
Updates Insights expectations after Graph removal. |
core/.../ProxyAuthentication.java |
Removes Graph-statement proxy authentication overload. |
core/.../SerializationUtils.java |
Updates geometry serialization assertions after removing Graph-only distance support. |
osgi-tests/.../CcmStagedReactor.java |
Stops enabling DSE Graph workloads. |
osgi-tests/.../BundleOptions.java |
Removes TinkerPop OSGi bundles. |
osgi-tests/.../MailboxActivator.java |
Removes Graph mailbox service registration. |
osgi-tests/.../application.conf |
Removes Graph configuration. |
integration-tests/.../application.conf |
Removes Graph integration-test configuration. |
manual/core/integration/README.md |
Removes TinkerPop integration instructions. |
manual/core/graalvm/README.md |
Removes Graph native-image guidance. |
manual/developer/request_execution/README.md |
Removes Graph execution-model examples. |
upgrade_guide/README.md |
Removes obsolete Graph dependency guidance. |
upgrade_guide/from_3x/README.md |
Updates request-processing guidance. |
changelog/README.md |
Records the Graph removal. |
Review details
- Files reviewed: 141/203 changed files
- Comments generated: 0
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| * | ||
| * <p>Value type: {@link String} | ||
| */ | ||
| GRAPH_TRAVERSAL_SOURCE("basic.graph.traversal-source"), |
There was a problem hiding this comment.
[Blocker] 🔴 Public enum constants removed with no deprecation cycle, in a 4th-component version (4.19.2.2-SNAPSHOT).
Beyond source compatibility: OptionsMap.SerializationProxy serializes Map<DriverOption, Object> directly and Java serializes enums by name, so any OptionsMap written by a <=4.19.2 driver fails to deserialize with InvalidObjectException: enum constant GRAPH_TRAVERSAL_SOURCE does not exist in class DseDriverOption. fillWithDriverDefaults put this constant unconditionally, so it is every serialized instance - which is the distributed-config case the Serializable contract exists for.
revapi will not catch it here: the verify log shows Comparing [] against [com.scylladb:java-driver-core:jar:4.19.2.2-SNAPSHOT], so the old artifact never resolves and core/revapi.json is inert.
Suggest keeping the constants @Deprecated and removing only the implementation, the way RESOLVE_CONTACT_POINTS was handled. Same for the ~20 TypedDriverOption fields, DseNodeMetric, the two DseSessionMetric constants, and dropping GraphSession/ReactiveGraphSession from CqlSession.
| // defined or in reference.conf) | ||
| : actualSupplier.get()); | ||
|
|
||
| DriverConfigLoader actualLoader = loader == null ? new DefaultDriverConfigLoader() : loader; |
There was a problem hiding this comment.
[Major] 🟠 This hands the caller's loader instance to the session. The old code always re-wrapped getConfigSupplier() in a fresh DefaultDriverConfigLoader, so every rule got its own TypesafeDriverConfig and its own SingleThreaded reload state.
With a shared loader - SessionRule.builder(res).withConfigLoader(l) alongside SessionUtils.newSession(res, l) - DefaultDriverConfigLoader.onDriverInit overwrites the loader's single singleThreaded field: the first session's periodic-reload future is orphaned and reload()/close() only ever affect the last session. Both sessions also read the same mutable config from getInitialConfig(), so a reload through one silently changes the other's getContext().getConfig().
Re-wrapping keeps the isolation while staying much shorter than the old block:
DriverConfigLoader actualLoader =
loader instanceof DefaultDriverConfigLoader
? new DefaultDriverConfigLoader(
((DefaultDriverConfigLoader) loader).getConfigSupplier())
: loader;Separately, loader == null ? new DefaultDriverConfigLoader() : loader duplicates the fallback already in SessionBuilder.buildAsync and drops its class-loader-aware defaultConfigLoader(programmaticArguments.getClassLoader()) variant - passing null straight through lets SessionBuilder apply the right default.
| import org.mockito.junit.MockitoJUnitRunner; | ||
|
|
||
| @RunWith(MockitoJUnitRunner.class) | ||
| public class GraphSchemaRefreshTest { |
There was a problem hiding this comment.
[Major] 🟠 The PR keeps the graph schema metadata - DseVertexMetadata, DseEdgeMetadata, graph_engine on DefaultDseKeyspaceMetadata, DefaultDseTableMetadata - but deletes its only unit test. This file imports nothing the PR removes; it would still compile and pass.
After the merge git grep DseVertexMetadata -- '*Test.java' is empty. What is left is KeyspaceGraphMetadataIT/TableGraphMetadataIT, which are DSE-only and do not run in this fork's CI. A SchemaRefresh regression on graph_engine change detection or on vertex/edge label diffing would ship with no test signal. Suggest keeping it until the metadata itself goes in #1025.
| * new execution methods: `CqlSession.executeContinuously*`. They have default implementations so | ||
| this doesn't break binary compatibility. You can just ignore them. | ||
| * new driver dependencies: ESRI and Reactive Streams. If you want to keep your classpath |
There was a problem hiding this comment.
[Major] 🟠 This rewrites history instead of recording the change. Users upgrading 4.3->4.4 on those driver versions did get executeGraph and a mandatory TinkerPop dependency; the guide now tells them otherwise. Same for the deleted #### DSE Graph dependencies are now optional section (L513-543) - that is the factual record of what 4.10.0 changed.
Meanwhile the change that does need documenting is not here: nothing states that executeGraph, DseNodeMetric, DseSessionMetric.GRAPH_*, the basic.graph/advanced.graph config trees and the graph-requests/graph-messages/graph-client-timeouts metrics are gone as of this version. An app setting advanced.graph.name gets a silently ignored key with no migration note to consult. Suggest leaving the historical sections intact and adding a new one for this release.
|
|
||
| ### 4.19.2 | ||
|
|
||
| - [improvement] Remove unused DSE Graph support and TinkerPop integration (#1028) |
There was a problem hiding this comment.
[Minor] 🟡 Two things. This lands under ### 4.19.2, which is already released, while the working version is 4.19.2.2-SNAPSHOT - anyone diffing 4.19.2 against its published artifact sees an entry that was not in it. And [improvement] puts a binary-breaking change (CqlSession no longer implements GraphSession, 20 DseDriverOption constants deleted) in the same bucket as "Remove unnecessary locking in DefaultNettyOptions" - that tag is the signal users scan before a patch upgrade to decide whether they have to recompile.
| * built-in codec. | ||
| */ | ||
| @Immutable | ||
| public class Distance extends DefaultGeometry { |
There was a problem hiding this comment.
[Minor] 🟡 The description scopes geometry out ("DSE geometry and schema metadata remain for follow-up cleanup"), but this drops Distance, DistanceSerializationProxy and DistanceTest, and strips the if (!(geometry instanceof Distance)) WKB guard from SerializationUtils. Behaviour-neutral today - Geo/GeoPredicate were the only consumers - but it leaves the geometry package half-gutted for #1025 with no changelog or description record, so whoever picks that up diffs against a state the issue does not describe. Either keep it for the geometry PR or say in the description that it went with its only callers.
Closes #1028. Part of #1025.
Remove unsupported DSE Graph APIs, request processors, serializers, metrics, tests, OSGi fixtures, documentation, and the TinkerPop/JavaTuples dependency chain. DSE geometry and schema metadata remain for follow-up cleanup.
Validation:
mvn -pl core,metrics/micrometer,metrics/microprofile,test-infra test— 3,724 tests passedmvn -pl integration-tests,osgi-tests -am -DskipTests test-compilemvn -DskipTests install— full 17-module reactor