Skip to content

CASSJAVA-124: Add GRACEFUL_DISCONNECT support (CEP-59) - #2091

Open
Shanzita wants to merge 16 commits into
apache:trunkfrom
Shanzita:cep-59
Open

CASSJAVA-124: Add GRACEFUL_DISCONNECT support (CEP-59)#2091
Shanzita wants to merge 16 commits into
apache:trunkfrom
Shanzita:cep-59

Conversation

@Shanzita

@Shanzita Shanzita commented May 28, 2026

Copy link
Copy Markdown

Summary

Implements client-side support for CEP-59 (Graceful Disconnect – In-Band
Connection Draining) in the Java driver.

When a Cassandra node shuts down, it emits a GRACEFUL_DISCONNECT event to
subscribed connections. The driver:

  • Stops sending new requests on the affected connection
  • Drains in-flight requests to completion
  • Closes the whole pool to the draining node and fails over
  • Reconnects automatically via the normal reconnection policy

Design

  • Capability is negotiated per connection: every channel that intends to
    register for GRACEFUL_DISCONNECT sends OPTIONS and filters its REGISTER
    against its own SUPPORTED response. There is no driver-global capability
    flag, so mixed-version clusters (rolling upgrades) work correctly.
  • The internal GracefulDisconnectEvent is node-scoped: it means "this node
    is shutting down", regardless of whether it arrived on a pool connection or
    the control connection, and the pool drains accordingly.
  • If a server advertises the capability but rejects the REGISTER, the driver
    retries once without the event type instead of failing channel init
    (mixed-version / evolving-server tolerance).
  • advanced.connection.graceful-disconnect-enabled config option (default true).
  • graceful-disconnects (session) and pool.graceful-disconnects (node)
    counters, initialized in the Dropwizard, Micrometer and MicroProfile
    backends and incremented where events are received.

Testing

  • 3,552 core unit tests pass (new coverage: ProtocolInitHandler capability
    filtering and REGISTER-rejection degradation, node-scoped ChannelPool drain,
    metric increments, duplicate/late event tolerance)
  • New GracefulDisconnectIT: bounded 2-node ccm test that drains a node under
    steady query load and asserts the metric observed the event, queries fail
    over, and zero exceptions reach the application. Gated on Cassandra 7.0+, so
    it is skipped until a server with CEP-59 is available in CI.
  • Manually validated end-to-end against a CEP-59 server build
    (CASSANDRA-21191: [CEP-59] Implementation of In-Band Connection Draining (Graceful Disconnect) cassandra#4953) on 2-node and 3-node ccm clusters:
    nodetool drain mid-load → 0 disruptive exceptions, thousands of successful
    queries after the event, drained node goes DOWN cleanly, failover invisible
    to the application. The 3-node run exercised the event arriving on the
    control connection and a pool connection simultaneously.

Depends on: native-protocol 1.5.3-SNAPSHOT (Shanzita/native-protocol:cep-59,
the branch behind datastax/native-protocol#61; installed by install-snapshots.sh
until 1.5.3 is released)
JIRA: CASSJAVA-124

SiyaoIsHiding and others added 7 commits May 27, 2026 17:00
  - Handle GRACEFUL_DISCONNECT event in InFlightHandler to drain in-flight requests
  - Register for GRACEFUL_DISCONNECT on both control and data connections
  - Add graceful-disconnect-enabled config option (default true)
  - Detect server support from SUPPORTED response in ChannelFactory
  - Add GRACEFUL_DISCONNECTS metric enums
  - Add unit tests for InFlightHandler and ControlConnection
  - Clean up WIP debug logging

  Co-authored-by: Jane He <jane.he@datastax.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds CEP-59 graceful-disconnect support to the Java driver, including event handling, connection draining, reconnection, configuration, metrics, and test infrastructure.

Changes:

  • Adds capability detection and graceful-disconnect event registration.
  • Drains affected connections and reconnects automatically.
  • Adds configuration, metrics, unit tests, integration coverage, and Docker setup.

Reviewed changes

Copilot reviewed 18 out of 18 changed files in this pull request and generated 12 comments.

Show a summary per file
File Summary and final review notes
integration-tests/src/test/resources/logback-test.xml Adds integration-test logging. Moderate (4 votes): unconditionally enables DEBUG frame logging, increasing suite output and cost.
integration-tests/src/test/java/com/datastax/oss/driver/core/connection/GracefulDisconnectIT.java Adds integration coverage. Critical (4 votes): hardcodes ccm-cluster, has an unbounded loop, and does not trigger or assert graceful draining. Critical (1 vote): fails outside the Docker network.
docker-compose.yaml Adds the integration environment. Moderate (4 votes): uses author-specific absolute macOS mounts. Critical (1 vote): Maven commands cannot resolve reactor artifacts from a clean repository. Moderate (2 votes): uses Surefire’s -Dtest instead of Failsafe’s -Dit.test.
core/src/test/java/com/datastax/oss/driver/internal/core/control/ControlConnectionEventsTest.java Tests control event registration and processing.
core/src/test/java/com/datastax/oss/driver/internal/core/channel/InFlightHandlerTest.java Tests graceful request draining.
core/src/main/resources/reference.conf Documents and enables the configuration option.
core/src/main/java/com/datastax/oss/driver/internal/core/pool/ChannelPool.java Coordinates pool draining. Critical (4 votes): channel membership checks miss control-originated and pending-query close paths, leaving other pool channels open.
core/src/main/java/com/datastax/oss/driver/internal/core/control/ControlConnection.java Registers and processes control events.
core/src/main/java/com/datastax/oss/driver/internal/core/channel/ProtocolInitHandler.java Handles protocol initialization. Critical (3 votes): incorrectly checks the nested EVENTS capability map and loses capabilities on later channels.
core/src/main/java/com/datastax/oss/driver/internal/core/channel/InFlightHandler.java Initiates graceful shutdown for in-flight requests.
core/src/main/java/com/datastax/oss/driver/internal/core/channel/GracefulDisconnectEvent.java Defines the internal graceful-disconnect event.
core/src/main/java/com/datastax/oss/driver/internal/core/channel/ChannelFactory.java Detects server support. Moderate (2 votes): globally caches the first node’s capability, breaking mixed-version or rolling-upgrade clusters.
core/src/main/java/com/datastax/oss/driver/api/core/metrics/DefaultSessionMetric.java Adds a session metric. Moderate (3 votes): no updater initializes or increments the counter.
core/src/main/java/com/datastax/oss/driver/api/core/metrics/DefaultNodeMetric.java Adds a node metric. Moderate (3 votes): no updater initializes or increments the counter.
core/src/main/java/com/datastax/oss/driver/api/core/config/TypedDriverOption.java Adds typed configuration access.
core/src/main/java/com/datastax/oss/driver/api/core/config/OptionsMap.java Sets the default configuration value.
core/src/main/java/com/datastax/oss/driver/api/core/config/DefaultDriverOption.java Adds the configuration option.
bom/pom.xml Adds the native-protocol snapshot dependency. Critical (1 vote): snapshot installation uses a different branch/repository, so required protocol classes may be unavailable in clean builds.
Suppressed comments (8)

core/src/main/java/com/datastax/oss/driver/internal/core/channel/ChannelFactory.java:246

  • After the first successful connection, productType is set (even to UNKNOWN), so every subsequent channel is initialized with querySupportedOptions == false. ProtocolInitHandler.filterSupportedEventTypes() then sees a null OPTIONS_KEY and removes GRACEFUL_DISCONNECT; consequently only the first channel subscribes, and a control-channel reconnect loses the subscription. Query OPTIONS per channel or propagate the cached capability into the initializer instead of using the per-channel attribute.
            }

core/src/main/java/com/datastax/oss/driver/internal/core/channel/ChannelFactory.java:245

  • The native protocol SUPPORTED response lists event capabilities under the EVENTS entry; it does not add each event type as a key whose value contains true. This lookup therefore never detects a CEP-59 server, leaving isGracefulDisconnectSupported() false and preventing query pools from subscribing.
            if (!serverSupportsGracefulDisconnect && supportedOptions != null) {
              List<String> gdValues = supportedOptions.get(GracefulDisconnectEvent.EVENT_TYPE);
              if (gdValues != null && gdValues.contains("true")) {
                serverSupportsGracefulDisconnect = true;
              }

core/src/main/java/com/datastax/oss/driver/internal/core/channel/InFlightHandler.java:228

  • Unlike the normal event path below, this new callback invocation is not protected from exceptions. For example, EventBus can throw while scheduling listeners during shutdown; that propagates through channelRead into exceptionCaught, aborting in-flight requests and defeating graceful draining. Wrap this callback in the same try/catch used for other event callbacks.
        if (eventCallback != null) {
          eventCallback.onEvent(event);
        }

core/src/main/java/com/datastax/oss/driver/internal/core/pool/ChannelPool.java:530

  • Calling close() on every channel here still triggers onChannelCloseStarted, which removes channels and immediately starts the pool's Reconnection. That can create replacement connections to the same node while it is draining, allowing new requests or repeatedly reconnecting/draining before shutdown completes. Suppress pool reconnection for this node until the graceful drain has completed or the node is known down.
        // Close ALL channels in the pool gracefully to immediately stop accepting new requests.
        // When all channels are closed, the NodeStateManager will automatically set the node to
        // DOWN state, which will trigger the LoadBalancingPolicy to remove it from the live set.
        // The graceful close allows in-flight requests to complete before channels are fully
        // closed.

core/src/main/java/com/datastax/oss/driver/internal/core/pool/ChannelPool.java:525

  • The two new GRACEFUL_DISCONNECTS metrics are never incremented on this path (and are absent from the concrete metric-updater initializers), so enabling them exposes no disconnect count and the counters remain unavailable/zero. Update the appropriate node/session updaters when the event is handled and initialize the counters in all supported metrics implementations.
        LOG.info(
            "[{}] Received GRACEFUL_DISCONNECT on channel {}, closing all channels gracefully",
            logPrefix,
            affectedChannel);

core/src/main/java/com/datastax/oss/driver/internal/core/pool/ChannelPool.java:266

  • The capability flag is learned only after a connection completes, but this value is captured while constructing the pool, before the pool's own connections are opened. If the initial/control connection is to an older node, this remains false and the pool never requests GRACEFUL_DISCONNECT, so a later connection to a supported node cannot enable draining. Do not gate pool registration on this sticky global flag; discover support per channel.
      this.gracefulDisconnectEnabled =
          config
                  .getDefaultProfile()
                  .getBoolean(DefaultDriverOption.GRACEFUL_DISCONNECT_ENABLED, true)
              && channelFactory.isGracefulDisconnectSupported();

docker-compose.yaml:22

  • The compose file requires a private JFrog image; a fresh checkout or CI runner without registry credentials cannot pull it, so this added integration path is not reproducible. Use a public/published image or make the image configurable with documented setup.
    image: apache.jfrog.io/cassan-docker/apache/cassandra-java-driver-testing-ubuntu2204:latest

docker-compose.yaml:51

  • This setup runs non-interactively inside the container but invokes apt install without -y. If iproute2 is not already present, apt waits for confirmation or exits on EOF, so the setup never reaches ccm create and the health check remains false. Use a non-interactive apt-get install -y (with an update if needed).
        sudo apt install iproute2 &&

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread bom/pom.xml
SPECULATIVE_EXECUTIONS("speculative-executions"),
CONNECTION_INIT_ERRORS("errors.connection.init"),
AUTHENTICATION_ERRORS("errors.connection.auth"),
GRACEFUL_DISCONNECTS("pool.graceful-disconnects"),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done; the counter is now initialized in all three backends (DropwizardNodeMetricUpdater, MicrometerNodeMetricUpdater, MicroProfileNodeMetricUpdater) in 618a7f8, and incremented when a GRACEFUL_DISCONNECT event is received on one of the node's pooled connections (ChannelPool query-connection callback, 442561d). It's documented in reference.conf, covered by the zero-value assertions in the three metrics ITs and by ChannelPoolGracefulDisconnectTest, and the new GracefulDisconnectIT exercises the session-level counter end to end against a real drain. I also verified both counters increment during manual drain runs on 2- and 3-node ccm clusters.

Comment on lines +241 to +245
if (!serverSupportsGracefulDisconnect && supportedOptions != null) {
List<String> gdValues = supportedOptions.get(GracefulDisconnectEvent.EVENT_TYPE);
if (gdValues != null && gdValues.contains("true")) {
serverSupportsGracefulDisconnect = true;
}

@SiyaoIsHiding SiyaoIsHiding Aug 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be tracked per connection

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reworked in 442561d exactly along these lines — capability is now tracked per connection:

  • The global serverSupportsGracefulDisconnect flag is removed from ChannelFactory entirely.
  • Any channel that intends to register for GRACEFUL_DISCONNECT always runs the OPTIONS step, and ProtocolInitHandler filters the REGISTER against that channel's own SUPPORTED response — so in a mixed-version cluster each node negotiates independently, and a reconnect re-negotiates. When the feature is disabled in config there's no extra round-trip.
  • ChannelPool now requests the event based on config alone and relies on the per-channel filtering, so the first-contact-point ordering issue is gone.
  • If a server advertises the capability but rejects the REGISTER, the driver retries once without the event type instead of failing channel init.

Covered by ProtocolInitHandlerGracefulDisconnectTest (advertised / not advertised / advertised-as-false / rejection-retry cases) and verified in a 3-node manual drain run.

Comment on lines +206 to +210
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)) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Supported options should be tracked per connection

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 442561d together with the ChannelFactory rework (see my reply on that thread for the full design): the factory fallback is gone, channels that request GRACEFUL_DISCONNECT always run the OPTIONS step, and filterSupportedEventTypes consults only this channel's own SUPPORTED response — so later channels and reconnects each negotiate for themselves.

One correction on the EVENTS point: the CEP-59 server advertises a top-level GRACEFUL_DISCONNECT key in SUPPORTED, not an entry under EVENTS — see OptionsMessage.execute() and InitialConnectionHandler in apache/cassandra#4953 (the pre-STARTUP path even sends GRACEFUL_DISCONNECT: ["false"] when the feature is disabled, which the driver's parsing handles). So the key check itself was correct; the real issues were the caching and the skipped OPTIONS, both fixed now.

Comment thread docker-compose.yaml Outdated
Comment thread docker-compose.yaml Outdated
cassandra-net:
ipv4_address: 172.20.0.10
command: >
sh -c "mvn compile -DskipTests && mvn verify -pl integration-tests -Dtest=GracefulDisconnectIT"

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

File removed in 85e7c25. The real IT (c146d19) runs under Failsafe with the repo's ccm test infrastructure, so no manual selector invocation is needed.

<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"/>

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The whole file has been reverted to trunk in 85e7c25, so the Frame DEBUG logger (and all the other debug loggers added here) are gone — this suggestion is superseded by the revert.

@SiyaoIsHiding SiyaoIsHiding left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Preliminary review

Comment thread bom/pom.xml
<groupId>com.datastax.oss</groupId>
<artifactId>native-protocol</artifactId>
<version>1.5.2</version>
<version>1.5.3-SNAPSHOT</version>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to remember to change it to 1.5.3 after the release of the native protocol

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed — I'll bump this to 1.5.3 as soon as native-protocol releases (tracking it in my native-protocol PR, datastax/native-protocol#61).

Related finding while fixing CI: the build had never actually resolved this snapshot — ci/run-tests.sh wasn't running install-snapshots.sh at all, so every CI run failed at dependency resolution. That's fixed now (402d1bf, 4885827, 6b9ac8f) and CI installs the snapshot from the PR #61 branch. One heads-up: I initially pointed it at your fork's cep-59 branch (which the PR description referenced), but that copy has Frame.forResponse stubbed out with UnsupportedOperationException, which failed the graph unit tests — you may want to update or remove that branch so nothing else picks it up.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't worry about CI or install-snapshots.sh. You can revert these changes

<configuration>
<appender name="STDOUT" class="ch.qos.logback.core.ConsoleAppender">
<encoder>
<pattern>%d{HH:mm:ss.SSS} [%thread] %-5level %logger{36} - %msg%n</pattern>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes in this file should be reverted

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reverted in 85e7c25 — the file now matches trunk exactly.

Comment thread docker-compose.yaml Outdated
@@ -0,0 +1,79 @@
services:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file should be removed

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed in 85e7c25.

public class GracefulDisconnectIT {

@Test
public void should_opt_in_gracefully_disconnect() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please write real IT for graceful disconnect

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in c146d19: 2-node ccm cluster via CustomCcmRule, steady query load, nodetool drain on node 2 mid-load, bounded by awaitility timeouts. It asserts the graceful-disconnects session metric observed the event, queries keep succeeding after the drain (failover), and no exception reaches the application. It's gated with @BackendRequirement(minInclusive = "7.0") so it skips until a CEP-59-capable server is available in CI — I ran the same flow manually against a CASSANDRA-21191 server build on 2- and 3-node clusters (0 disruptive exceptions in both).

@Test
public void should_process_status_change_events() {
// Given
when(defaultProfile.getBoolean(DefaultDriverOption.GRACEFUL_DISCONNECT_ENABLED, true))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need add these for status change and topology change events' tests?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You were right to question this — those tests only exercise the event callback, never registration, so the stubs were unnecessary. Removed in 23835de; only the registration tests stub the option now.

THROTTLING_QUEUE_SIZE("throttling.queue-size"),
THROTTLING_ERRORS("throttling.errors"),
CQL_PREPARED_CACHE_SIZE("cql-prepared-cache-size"),
GRACEFUL_DISCONNECTS("graceful-disconnects"),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need integration tests and manual testing for metrics

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and the actual implementation of incrementing the metric

@Shanzita Shanzita Aug 26, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All three parts are done now:

  • Incrementing (442561d): the session counter increments wherever a GRACEFUL_DISCONNECT event is received — the pool's query-connection callback and the control connection. The node counter (pool.graceful-disconnects) increments for events on that node's pooled connections.
  • Initialization (618a7f8): both counters are initialized in all three backends (Dropwizard, Micrometer, MicroProfile) and documented in reference.conf; the three metrics ITs assert they exist as zero-valued counters, and ControlConnectionEventsTest / ChannelPoolGracefulDisconnectTest verify the increments at the unit level.
  • Integration + manual testing: the new GracefulDisconnectIT (c146d19) asserts this counter goes above zero during a real nodetool drain under load. I also verified both counters manually against a CASSANDRA-21191 server build on 2-node and 3-node ccm clusters — the drain runs finished with the event observed, counters incremented, and 0 disruptive exceptions.

Addresses review: debug logging changes in the shared IT config should not ship with the PR.
Removes the global flag from ChannelFactory: any channel registering for GRACEFUL_DISCONNECT now runs OPTIONS and filters REGISTER against its own SUPPORTED response, so mixed-version clusters negotiate per node. GracefulDisconnectEvent becomes node-scoped (fixes the pool membership check missing control-connection events and the closingChannels race, and removes a race where an event before setChannel was dropped). Event callbacks on the drain path are now exception-guarded. Increments the GRACEFUL_DISCONNECTS metrics where events are received.
The status/topology/schema processing tests never exercise registration, so the GRACEFUL_DISCONNECT_ENABLED stubs were unnecessary. Also asserts the session metric increments when the control connection processes the event.
Initializes the session and node counters in the Dropwizard, Micrometer and MicroProfile updaters, documents both metrics in reference.conf, and covers them in the three metrics ITs.
Bounded 2-node ccm test: steady query load, nodetool drain mid-load, asserts the session metric observed the event, queries fail over, and zero exceptions reach the application. Gated on Cassandra 7.0+ so it skips where the server lacks CEP-59.
Clean builds need the CEP-59 protocol types, which are not on the native-protocol default branch yet. TODO in the script to revert once 1.5.3 is released.
ci/run-tests.sh never ran install-snapshots.sh, so Jenkins could not resolve native-protocol:1.5.3-SNAPSHOT (CI has failed on dependency resolution since the PR was opened). Also drops the tee to /dev/tty, which breaks in CI containers where no tty is allocated.
…branch

The SiyaoIsHiding fork's cep-59 branch has Frame.forResponse stubbed out (throws UnsupportedOperationException), which broke hundreds of graph unit tests in CI once dependency resolution was fixed. The Shanzita fork's cep-59 branch (the branch behind datastax/native-protocol PR apache#61) has the working implementation and matches what was tested locally.
Install the native-protocol snapshot unconditionally: the previous mvn probe wrote a resolution-failure marker into the local repository when the artifact was not yet installed, which then blocked the main build in one matrix cell (build apache#5). Fail the build immediately if the install fails, clone into a unique temp dir to avoid collisions between concurrent builds, and pass -U so stale failure markers from earlier builds are ignored.
@Shanzita

Copy link
Copy Markdown
Author

I've addressed the first round of review — thank you both for the feedback. Summary of the changes:

  • Per-connection capability tracking (442561d): removed the global flag; every channel that registers for GRACEFUL_DISCONNECT sends OPTIONS and filters REGISTER against its own SUPPORTED response, so mixed-version clusters negotiate per node. The internal event is now node-scoped, which also fixes the pool-membership issues flagged in ChannelPool.
  • Metrics implemented end to end (618a7f8): initialized in Dropwizard/Micrometer/MicroProfile for session + node, incremented where events are received, documented in reference.conf, covered by unit tests and the metrics ITs.
  • Real GracefulDisconnectIT (c146d19): bounded 2-node ccm test — drain under load, asserts the metric saw the event, failover works, zero exceptions reach the application. Gated on 7.0+ so it skips until a CEP-59 server is in CI.
  • CI fixes (402d1bf, 4885827, 6b9ac8f): CI had never compiled this PR — ci/run-tests.sh never installed the native-protocol snapshot. It now installs it from my native-protocol PR branch (Add GRACEFUL_DISCONNECT event support (CEP-59) datastax/native-protocol#61).
  • Reverted logback-test.xml and removed docker-compose.yaml (85e7c25), cleaned up the test stubs (23835de).

Manually validated end to end against a server built from apache/cassandra#4953 on 2-node and 3-node ccm clusters: nodetool drain under steady load → 0 disruptive exceptions, thousands of successful queries after the event, drained node goes DOWN cleanly, failover invisible to the application. The 3-node run exercised the event arriving on the control connection and a pool connection simultaneously. All 3,552 core unit tests pass.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 32 out of 32 changed files in this pull request and generated 3 comments.

Comment on lines +527 to +530
// Reconnection will start automatically once all channels are closed.
for (DriverChannel channel : channels) {
channel.close();
}
Comment on lines +119 to +122
} finally {
stopped.set(true);
load.join(TimeUnit.SECONDS.toMillis(10));
}
Comment thread install-snapshots.sh
Comment on lines +37 to +44
# Clone into a unique directory so concurrent builds on the same host cannot collide.
CLONE_DIR=$(mktemp -d)/$(basename ${URL} .git)
git clone --depth 1 --branch ${BRANCH} ${URL} ${CLONE_DIR}
(
cd ${CLONE_DIR}
mvn -B install -DskipTests
)
rm -rf ${CLONE_DIR}
@SiyaoIsHiding

Copy link
Copy Markdown
Contributor

A tricky design decision to make: when a connection receives graceful disconnect event, should it propagate to other connections on the same node?

If propagates to other connections:

Pro: the event says the node is going down, so other connections on this node should drain, too, it makes sense.
Con: no clean way to implement. The PR as it is right now, has all connections firing graceful disconnect event to the event bus, and all query connections listens for graceful disconnect event. So, if there are n connections on the same node, it's handled n*n times. Although closing channels are mostly idempotent, closing a channel that is already closing is no-op, it's still very error-prone.

  • The metrics can be incremented multiple times.
  • What if one connection receives the event first, close the channel, then the node comes back up, so it opens again, but then finally the other connection on the same node finally receives the event cuz it was delayed, then it will close all connections on this node again.
  • The above two problems are hard to solve because there isn't an easy way to tell which graceful disconnect events are from the same episode of a node shutting down.
    But this is exactly the problem CEP-59 trying to solve - it's aimed to solve how gossip-based events can be duplicated and unreliable because it's out of band, now the graceful disconnect event is again out-of-band, duplicated and unreliable.

if the handling of graceful disconnect stays local to the connection

Pro: very easy and clean to implement. Just as how Graceful disconnect is designed, "an in-band event local to the connection".
Con: If one connection somehow didn't receive the graceful disconnect event, it won't be notified from another connection to the same node that got the event, and it can timeout just like connections without cep-59 enabled. I think this con is trivial, because all connections between a client that supports cep-59 and a server that supports cep-59 should have registered for graceful_disconnect, so the likelihood of one connection gets the event but others do not is pretty low.

So I prefer the handling of graceful disconnect stays local to the connection but not propagated.
@absurdfarce what do you think?

@SiyaoIsHiding SiyaoIsHiding left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really great work!! Looks so much better now!
Yet to review the tests. I will continue tomorrow.

Comment thread install-snapshots.sh
@@ -6,36 +6,42 @@
# to you under the Apache License, Version 2.0 (the

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OMG I didn't even know this file existed. This file is obsolete and we should probably delete it in another ticket.
We don't need to worry about install-snapshots.sh or ci/run_tests.sh in this PR. We will get the native protocol release out soon, so the CI will pass.

Comment thread bom/pom.xml
<groupId>com.datastax.oss</groupId>
<artifactId>native-protocol</artifactId>
<version>1.5.2</version>
<version>1.5.3-SNAPSHOT</version>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't worry about CI or install-snapshots.sh. You can revert these changes

*
* <p>This is part of CEP-59: Graceful Disconnect – In-Band Connection Draining for Node Shutdown.
*/
@Immutable

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pls refer to TopologyEvent, refactor this class to under the package com.datastax.oss.driver.internal.core.metadata, and remove GracefulDisconnectEvent.EVENT_TYPE, and change all usages of GracefulDisconnectEvent.EVENT_TYPE to ProtocolConstants.EventType.GRACEFUL_DISCONNECT.
Graceful disconnect is just another event just like topology event and status change event.

/** The node that sent the graceful disconnect event. */
public final Node node;

public GracefulDisconnectEvent(Node node) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO for myself:
check memory leak possibilities.

public GracefulDisconnectEvent(Node node) {
this.node = node;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refer to TopologyEvent add

  @Override
  public int hashCode() {
    return Objects.hash(this.node);
  }

} else {
LOG.debug("[{}] Processing incoming event {}", logPrefix, eventMessage);
Event event = (Event) eventMessage;
LOG.debug("[{}] Processing incoming event {}", logPrefix, eventMessage);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pls revert the reversion of these two lines

this.channelFactory = context.getChannelFactory();
this.eventBus = context.getEventBus();
this.sessionMetricUpdater = context.getMetricsFactory().getSessionUpdater();
// Whether graceful disconnect is enabled in the configuration. Server-side support is

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a discussion on the mailing list about AI generated redundant comments. It'd be great if we scrutinize our comments. I think these are one of those unnecessary comments.

context
.getMetricsFactory()
.getSessionUpdater()
.incrementCounter(DefaultSessionMetric.GRACEFUL_DISCONNECTS, null);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not increment DefaultNodeMetric.GRACEFUL_DISCONNECTS?

return;
}
LOG.info(
"[{}] Received GRACEFUL_DISCONNECT for {}, closing all channels gracefully",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"[{}] Received GRACEFUL_DISCONNECT for {}, closing all channels gracefully",
"[{}] Received GRACEFUL_DISCONNECT for {}, closing all channels for this node gracefully",

// DOWN state, which will trigger the LoadBalancingPolicy to remove it from the live set.
// The graceful close allows in-flight requests to complete before channels are fully
// closed.
// Reconnection will start automatically once all channels are closed.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only this line is needed here as comments

      // The graceful close allows in-flight requests to complete before channels are fully

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants