Skip to content

CASSANDRA-21191: [CEP-59] Implementation of In-Band Connection Draining (Graceful Disconnect) - #4953

Open
AshenScribe wants to merge 3 commits into
apache:trunkfrom
AshenScribe:feature/cep-59/metrics
Open

CASSANDRA-21191: [CEP-59] Implementation of In-Band Connection Draining (Graceful Disconnect)#4953
AshenScribe wants to merge 3 commits into
apache:trunkfrom
AshenScribe:feature/cep-59/metrics

Conversation

@AshenScribe

Copy link
Copy Markdown
Contributor

@AshenScribe AshenScribe left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

StorageService.drain() was synchronized on the same monitor as drain(boolean). Pre-CEP-59, drain() called drain(false) directly on the same thread, so reentrancy made this safe. Now drain() hands a callback to gracefulDisconnect(...), which invokes it asynchronously (Netty close-listener or scheduler thread) — so drain(false) can run on a different thread than the one blocked in drain()'s await(). Since that thread still holds the monitor while waiting, drain(false) can never acquire it → deadlock whenever a client is still connected at drain time.

Fix: drop synchronized from drain(), use a dedicated ReentrantLock scoped only to drain() (preserves "one drain at a time"), leave drain(boolean)'s own synchronized untouched.

Copilot AI 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.

Pull request overview

Implements CEP-59 graceful native-protocol disconnection for controlled Cassandra shutdowns.

Changes:

  • Adds capability advertisement, event registration, and connection draining.
  • Integrates draining with shutdown operations and metrics.
  • Adds configuration, documentation, and tests.

Reviewed changes

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

Show a summary per file
File Description
test/unit/org/apache/cassandra/service/StorageServiceTest.java Tests graceful-disconnect settings.
test/unit/org/apache/cassandra/config/DatabaseDescriptorTest.java Tests configuration defaults and updates.
test/unit/org/apache/cassandra/concurrent/DebuggableScheduledThreadPoolExecutorTest.java Propagates the new drain exception.
test/distributed/org/apache/cassandra/distributed/test/GracefulDisconnectTest.java Adds distributed feature tests.
src/java/org/apache/cassandra/transport/SimpleClient.java Handles graceful-disconnect events.
src/java/org/apache/cassandra/transport/Server.java Tracks subscribers and stops accepting connections.
src/java/org/apache/cassandra/transport/messages/StartupMessage.java Defines the capability key.
src/java/org/apache/cassandra/transport/messages/OptionsMessage.java Advertises feature support.
src/java/org/apache/cassandra/transport/InitialConnectionHandler.java Advertises support before negotiation.
src/java/org/apache/cassandra/transport/Event.java Defines the protocol event.
src/java/org/apache/cassandra/tools/nodetool/Drain.java Handles drain timeout errors.
src/java/org/apache/cassandra/tools/NodeProbe.java Propagates drain timeouts.
src/java/org/apache/cassandra/service/StorageServiceMBean.java Exposes settings and timeout contract.
src/java/org/apache/cassandra/service/StorageService.java Orchestrates graceful shutdown.
src/java/org/apache/cassandra/service/NativeTransportService.java Exposes subscribed channels.
src/java/org/apache/cassandra/metrics/ClientMetrics.java Adds draining metrics.
src/java/org/apache/cassandra/config/DatabaseDescriptor.java Exposes configuration accessors.
src/java/org/apache/cassandra/config/Config.java Defines feature configuration.
doc/modules/cassandra/pages/managing/operating/graceful_disconnect.adoc Documents operation and compatibility.
conf/cassandra.yaml Documents stable configuration defaults.
conf/cassandra_latest.yaml Enables the feature in latest configuration.
Suppressed comments (1)

src/java/org/apache/cassandra/transport/SimpleClient.java:85

  • Removing this suppression makes Netty Promise fail the IllegalImport check in .build/checkstyle.xml:104. Restore the established import-level suppression.
import io.netty.util.concurrent.Promise;

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

import java.util.UUID;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.CopyOnWriteArrayList;
import java.util.concurrent.CountDownLatch;
import java.util.concurrent.BlockingQueue;
import java.util.concurrent.ConcurrentLinkedQueue;
import java.util.concurrent.SynchronousQueue; // checkstyle: permit this import
import java.util.concurrent.SynchronousQueue;
supportedOptions.put(StartupMessage.CQL_VERSION, cqlVersions);
supportedOptions.put(StartupMessage.COMPRESSION, compressions);
supportedOptions.put(StartupMessage.PROTOCOL_VERSIONS, ProtocolVersion.supportedVersions());
supportedOptions.put(StartupMessage.GRACEFUL_DISCONNECT, List.of(String.valueOf(DatabaseDescriptor.getGracefulDisconnectEnabled())));

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

+1

Comment on lines +3925 to +3926
if (!drainComplete.await(DatabaseDescriptor.getGracefulDisconnectGracePeriod(), MILLISECONDS))
throw new TimeoutException("Timed out waiting for drain to complete after graceful disconnect");
Comment on lines +3994 to +3997
if (connectedChannels.decrementAndGet() == 0)
{
timeoutTask.cancel(false);
runOnceAction.run();
Comment on lines +146 to +148
public boolean graceful_disconnect_enabled = false;

public volatile DurationSpec.LongMillisecondsBound graceful_disconnect_grace_period = new DurationSpec.LongMillisecondsBound(5000);
@Test
public void testGracefulDisconnectEnabled()
{
Assertions.assertThat(StorageService.instance.getGracefulDisconnectEnabled()).isFalse();
Comment on lines +3964 to +3965
AtomicBoolean actionStarted = new AtomicBoolean(false);
AtomicInteger connectedChannels = new AtomicInteger(channelGroup.size());
Comment on lines +169 to +173
if (bindChannel != null && bindChannel.isOpen())
{
logger.info("Stopping native transport acceptor on {}", bindChannel.localAddress());
// syncUninterruptibly ensures we wait for the port to actually close
bindChannel.close().syncUninterruptibly();

== Solution

Introduce an in-band signal — `GRACEFUL_DISCONNECT` — so that the server can notify clients (that have subscribed) connection before closing it. This gives drivers time to:

@SiyaoIsHiding SiyaoIsHiding 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.

Preliminary review, yet to dig into the core mechanism.
The most convincing test is to put together a client-server scenario, where it throws an error when graceful disconnect is disabled, and does not throw an error when graceful disconnect is enabled. This is the whole purpose of CEP-59.
But the existing tests are only about configs, whether server sends event or start draining, etc. We need the test to prove the error is gone.
If the existing integration test harness is not enough to put together such scenario, then a manual testing is needed.

connection.setCompressor(Compressor.LZ4Compressor.instance);
}
if (version.isGreaterOrEqualTo(ProtocolVersion.V5))
options.put(StartupMessage.GRACEFUL_DISCONNECT, "GRACEFUL_DISCONNECT");

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

graceful disconnect is opted in by REGISTER, not in startup message

supportedOptions.put(StartupMessage.CQL_VERSION, cqlVersions);
supportedOptions.put(StartupMessage.COMPRESSION, compressions);
supportedOptions.put(StartupMessage.PROTOCOL_VERSIONS, ProtocolVersion.supportedVersions());
supportedOptions.put(StartupMessage.GRACEFUL_DISCONNECT, List.of(String.valueOf(DatabaseDescriptor.getGracefulDisconnectEnabled())));

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

+1

}
});
if (!drainComplete.await(DatabaseDescriptor.getGracefulDisconnectGracePeriod(), MILLISECONDS))
throw new TimeoutException("Timed out waiting for drain to complete after graceful disconnect");

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Why does drain throws a TimeoutException when graceful period passes? it should at most log a warning and proceed IMO

@VisibleForTesting
Gauge<Integer> connectedNativeClients;

public AtomicInteger connectionsDraining;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

We need tests for metrics too

@SiyaoIsHiding SiyaoIsHiding changed the title cep 59 CASSANDRA-21191: [CEP-59] Implementation of In-Band Connection Draining (Graceful Disconnect) Aug 14, 2026
@AshenScribe

Copy link
Copy Markdown
Contributor Author

@SiyaoIsHiding I need a java driver supporting cep 59. I am unable to get one, so further testing can't be done by me.

@SiyaoIsHiding

Copy link
Copy Markdown

@AshenScribe I sent you a working java driver and a working native protocol on Feb 25, I also hopped on an half hour meeting with you around the same time when I showed you how to do necessary end-to-end testing.

@AshenScribe

Copy link
Copy Markdown
Contributor Author

Hi @SiyaoIsHiding yes I remember that meeting. I'll do so.

As of drivers, since shanzita took over the java driver, I was waiting for her branch of native protocol, as that will be the official code to be merged now, which I received just yesterday.

@SiyaoIsHiding

Copy link
Copy Markdown

She didn't have a full server side implementation to test against until last month, either. You two needs each other to do end-to-end testing, so you both will need to test against WIP code, and you should expect changes through out the PR review process.

import java.util.UUID;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.CopyOnWriteArrayList;
import org.apache.cassandra.utils.concurrent.CountDownLatch

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.

I think you missed a semicolon at the end of this import.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yup, I also skipped the import style check skip patching it now.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Also wait some moment before reviewing the StorageService class, I am going to change the approach there.

# Conflicts:
#	src/java/org/apache/cassandra/service/StorageService.java
@AshenScribe
AshenScribe force-pushed the feature/cep-59/metrics branch from bdeaf90 to c0641da Compare August 20, 2026 12:59
@AshenScribe

Copy link
Copy Markdown
Contributor Author

added new logic for draining.
Todo

  • Write comprehensive test cases for GracefulDisconnectLifecycle, it contains basic ones only
  • Write comprehensive test cases for GracefulDisconnect feature, it contains basic ones only
  • Write comprehensive test cases for new metrics
  • Do manual testing using new java driver and native-protocol and this PR. Current test include draining with graceful_disconnect_enabled set to true and false and used cqlsh to test legacy behaviour.

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.

4 participants