You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by "Copilot (via GitHub)" <gi...@apache.org> on 2026/08/24 19:05:38 UTC
Re: [PR] CASSJAVA-124: Add GRACEFUL_DISCONNECT support (CEP-59) [cassandra-java-driver]
Copilot commented on code in PR #2091:
URL: https://github.com/apache/cassandra-java-driver/pull/2091#discussion_r3846599079
##########
bom/pom.xml:
##########
@@ -78,7 +78,7 @@
<dependency>
<groupId>com.datastax.oss</groupId>
<artifactId>native-protocol</artifactId>
- <version>1.5.2</version>
+ <version>1.5.3-SNAPSHOT</version>
Review Comment:
This introduces a dependency on a snapshot containing the CEP-59 protocol types, but `install-snapshots.sh` clones the public native-protocol default branch rather than the fork/`cep-59` branch specified in the PR description. On a clean build the required `GracefulDisconnectEvent` classes may therefore be absent and resolution or compilation can fail. Update snapshot installation/repository and branch together, or use a released artifact.
##########
core/src/main/java/com/datastax/oss/driver/api/core/metrics/DefaultNodeMetric.java:
##########
@@ -51,6 +51,7 @@ public enum DefaultNodeMetric implements NodeMetric {
SPECULATIVE_EXECUTIONS("speculative-executions"),
CONNECTION_INIT_ERRORS("errors.connection.init"),
AUTHENTICATION_ERRORS("errors.connection.auth"),
+ GRACEFUL_DISCONNECTS("pool.graceful-disconnects"),
Review Comment:
This adds a counter to the public node metric list, but none of the concrete node metric updaters initialize it and no code increments it when a graceful-disconnect event is handled. With all node metrics enabled, the existing metrics integration tests expect a registry entry for every enum value, so this metric is absent and cannot report events. Add initialization in each backend and increment it during event handling.
##########
core/src/main/java/com/datastax/oss/driver/internal/core/channel/ChannelFactory.java:
##########
@@ -232,6 +238,12 @@ private void connect(
ConsistencyLevel.LOCAL_QUORUM.name()));
}
}
+ if (!serverSupportsGracefulDisconnect && supportedOptions != null) {
+ List<String> gdValues = supportedOptions.get(GracefulDisconnectEvent.EVENT_TYPE);
+ if (gdValues != null && gdValues.contains("true")) {
+ serverSupportsGracefulDisconnect = true;
+ }
Review Comment:
`serverSupportsGracefulDisconnect` is cached globally from the first `SUPPORTED` response. During a rolling upgrade or mixed-version cluster, if the first contact point is an older node, this remains false and `ChannelPool` never subscribes on newer nodes even when they advertise the feature. The capability needs to be tracked per endpoint, or registration must be enabled and filtered per channel.
This issue also appears on line 241 of the same file.
##########
docker-compose.yaml:
##########
@@ -0,0 +1,79 @@
+services:
+ java-client:
+ image: maven:3.9-eclipse-temurin-8
+ container_name: java-client
+ working_dir: /app
+ depends_on:
+ ccm-cluster:
+ condition: service_healthy
+ cap_add:
+ - NET_ADMIN
+ - NET_RAW
+ volumes:
+ - .:/app
+ - /Users/siyaohe/.m2/repository:/root/.m2/repository
Review Comment:
These bind mounts use author-specific absolute macOS paths (and the Cassandra checkout mount below has the same issue), so this compose setup fails or mounts empty directories on other hosts and CI. Use environment variables with documented defaults or repo-relative/buildable inputs.
##########
docker-compose.yaml:
##########
@@ -0,0 +1,79 @@
+services:
+ java-client:
+ image: maven:3.9-eclipse-temurin-8
+ container_name: java-client
+ working_dir: /app
+ depends_on:
+ ccm-cluster:
+ condition: service_healthy
+ cap_add:
+ - NET_ADMIN
+ - NET_RAW
+ volumes:
+ - .:/app
+ - /Users/siyaohe/.m2/repository:/root/.m2/repository
+ networks:
+ cassandra-net:
+ ipv4_address: 172.20.0.10
+ command: >
+ sh -c "mvn compile -DskipTests && mvn verify -pl integration-tests -Dtest=GracefulDisconnectIT"
Review Comment:
`mvn compile` does not install the reactor artifacts, while the subsequent `verify -pl integration-tests` excludes the upstream driver modules because `-am` is missing. With a clean Maven repository this second invocation cannot resolve the locally built driver artifacts; use one reactor invocation with `-am` or install before selecting the integration module.
##########
integration-tests/src/test/java/com/datastax/oss/driver/core/connection/GracefulDisconnectIT.java:
##########
@@ -0,0 +1,42 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package com.datastax.oss.driver.core.connection;
+
+import com.datastax.oss.driver.api.core.CqlSession;
+import java.net.InetSocketAddress;
+import org.junit.Test;
+
+public class GracefulDisconnectIT {
+
+ @Test
+ public void should_opt_in_gracefully_disconnect() {
+ try (CqlSession session =
+ CqlSession.builder()
+ .addContactPoint(new InetSocketAddress("ccm-cluster", 9042))
Review Comment:
This class matches the normal Failsafe `*IT` discovery pattern but hardcodes the Docker-only hostname `ccm-cluster`. Regular integration-test runs outside this compose network cannot resolve or connect to it, so the suite fails before exercising graceful disconnect. Use the existing test-infrastructure contact points or isolate this test to the compose harness.
##########
integration-tests/src/test/resources/logback-test.xml:
##########
@@ -24,14 +24,17 @@
<pattern>%d{HH:mm:ss.SSS} [%thread] %-5level %logger{36} - %msg%n</pattern>
</encoder>
</appender>
- <root level="ERROR">
+ <root level="INFO">
<appender-ref ref="STDOUT"/>
</root>
<logger name="org.apache.directory" level="${adsServerLevel:-ERROR}"/>
- <logger name="com.datastax.oss.driver" level="${ossDriverLevel:-ERROR}"/>
- <logger name="com.datastax.dse.driver" level="${dseDriverLevel:-ERROR}"/>
- <logger name="com.datastax.oss.simulacron" level="${simulacronLevel:-ERROR}"/>
<logger name="com.datastax.oss.driver.api.testinfra.ccm" level="${ccmLevel:-INFO}"/>
+ <logger name="com.datastax.oss.driver.internal.core.pool.ChannelPool" level="DEBUG"/>
+ <logger name="com.datastax.oss.driver.internal.core.control.ControlConnection" level="DEBUG"/>
+ <logger name="com.datastax.oss.driver.internal.core.channel.ProtocolInitHandler" level="DEBUG"/>
+ <logger name="com.datastax.oss.driver.internal.core.channel.ChannelFactory" level="DEBUG"/>
+ <logger name="com.datastax.oss.protocol.internal.Frame" level="DEBUG"/>
Review Comment:
This unconditionally enables DEBUG for the protocol `Frame` logger in the shared integration-test configuration. Because every request and response is a frame, a full suite will emit per-frame logs and greatly increase output and test cost. Keep this behind a configurable level or a dedicated diagnostic configuration.
##########
core/src/main/java/com/datastax/oss/driver/api/core/metrics/DefaultSessionMetric.java:
##########
@@ -32,6 +32,7 @@ public enum DefaultSessionMetric implements SessionMetric {
THROTTLING_QUEUE_SIZE("throttling.queue-size"),
THROTTLING_ERRORS("throttling.errors"),
CQL_PREPARED_CACHE_SIZE("cql-prepared-cache-size"),
+ GRACEFUL_DISCONNECTS("graceful-disconnects"),
Review Comment:
This adds a counter to the public session metric list, but none of the concrete session metric updaters initialize it and no code increments it when a graceful-disconnect event is handled. With all session metrics enabled, the existing metrics integration tests expect a registry entry for every enum value, so this metric is absent and cannot report events. Add initialization in each backend and increment it during event handling.
##########
core/src/main/java/com/datastax/oss/driver/internal/core/channel/ProtocolInitHandler.java:
##########
@@ -183,12 +185,36 @@ Message getRequest() {
case AUTH_RESPONSE:
return request = new AuthResponse(authResponseToken);
case REGISTER:
- return request = new Register(options.eventTypes);
+ return request = new Register(filterSupportedEventTypes());
default:
throw new AssertionError("unhandled step: " + step);
}
}
+ /**
+ * Filters the requested event types to only include those supported by the server.
+ *
+ * <p>Specifically, GRACEFUL_DISCONNECT is only included if the server advertises support for it
+ * in the SUPPORTED message response.
+ */
+ private List<String> filterSupportedEventTypes() {
+ List<String> filteredEventTypes = new ArrayList<>(options.eventTypes);
+
+ // Check if GRACEFUL_DISCONNECT is in the requested event types
+ if (filteredEventTypes.contains(GracefulDisconnectEvent.EVENT_TYPE)) {
+ // Get the supported options from the channel attribute (set during OPTIONS step)
+ Map<String, List<String>> supportedOptions = channel.attr(DriverChannel.OPTIONS_KEY).get();
+
+ // Only include GRACEFUL_DISCONNECT if the server supports it
+ if (supportedOptions == null
+ || !supportedOptions.containsKey(GracefulDisconnectEvent.EVENT_TYPE)) {
Review Comment:
The event capabilities in `SUPPORTED` are listed under the `EVENTS` entry, so even the first channel (where this attribute is populated) fails `containsKey(GRACEFUL_DISCONNECT)` and drops the event. Later channels and reconnects also see `null` because OPTIONS is queried only for the first channel. Use the factory's cached capability or propagate the supported `EVENTS` list instead of checking this per-channel map.
##########
core/src/main/java/com/datastax/oss/driver/internal/core/pool/ChannelPool.java:
##########
@@ -474,6 +503,74 @@ private void onConfigChanged(@SuppressWarnings("unused") ConfigChangeEvent event
resize(distance);
}
+ private void onGracefulDisconnect(GracefulDisconnectEvent event) {
+ assert adminExecutor.inEventLoop();
+ // Only handle events for channels belonging to this pool's node
+ if (!event.node.equals(node)) {
+ return;
+ }
+ DriverChannel affectedChannel = event.channel;
+ // Check if this channel belongs to this pool
+ boolean channelFound = false;
+ for (DriverChannel channel : channels) {
+ if (channel == affectedChannel) {
+ channelFound = true;
+ break;
+ }
Review Comment:
This membership check misses both relevant event paths: `ControlConnection` publishes an event carrying the control channel, which is never in this pool, and for a pending query `startGracefulShutdown` completes `closeStartedFuture` before the callback, so `onChannelCloseStarted` moves the query channel to `closingChannels` before this queued handler runs. `channelFound` is therefore false and the remaining pool channels are not closed. Use a node-scoped drain signal or include the closing/control-originated cases.
##########
integration-tests/src/test/java/com/datastax/oss/driver/core/connection/GracefulDisconnectIT.java:
##########
@@ -0,0 +1,42 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package com.datastax.oss.driver.core.connection;
+
+import com.datastax.oss.driver.api.core.CqlSession;
+import java.net.InetSocketAddress;
+import org.junit.Test;
+
+public class GracefulDisconnectIT {
+
+ @Test
+ public void should_opt_in_gracefully_disconnect() {
+ try (CqlSession session =
+ CqlSession.builder()
+ .addContactPoint(new InetSocketAddress("ccm-cluster", 9042))
+ .withLocalDatacenter("datacenter1")
+ .build()) {
+ while (true) {
+ session.execute("SELECT * FROM system.local");
+ Thread.sleep(10);
Review Comment:
This class is discovered by Failsafe as `*IT` and runs in the normal integration-test suite, but it hard-codes the Docker-only `ccm-cluster` hostname; outside that compose network the session cannot connect. Even in the compose network, the unconditional loop never terminates, and the test only sends `SELECT` requests without triggering a node drain or asserting draining/reconnection. Make it a bounded, orchestrated test and isolate it from the default suite if necessary.
##########
docker-compose.yaml:
##########
@@ -0,0 +1,79 @@
+services:
+ java-client:
+ image: maven:3.9-eclipse-temurin-8
+ container_name: java-client
+ working_dir: /app
+ depends_on:
+ ccm-cluster:
+ condition: service_healthy
+ cap_add:
+ - NET_ADMIN
+ - NET_RAW
+ volumes:
+ - .:/app
+ - /Users/siyaohe/.m2/repository:/root/.m2/repository
+ networks:
+ cassandra-net:
+ ipv4_address: 172.20.0.10
+ command: >
+ sh -c "mvn compile -DskipTests && mvn verify -pl integration-tests -Dtest=GracefulDisconnectIT"
Review Comment:
The `integration-tests` module runs these `*IT` classes with Maven Failsafe, but `-Dtest` is the Surefire selector; Failsafe uses `-Dit.test`. This command therefore does not reliably select `GracefulDisconnectIT` and can run the entire integration suite. Use the Failsafe selector or an explicit include.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org