CASSANDRA-21191: [CEP-59] Implementation of In-Band Connection Draining (Graceful Disconnect) - #4953
CASSANDRA-21191: [CEP-59] Implementation of In-Band Connection Draining (Graceful Disconnect)#4953AshenScribe wants to merge 3 commits into
Conversation
AshenScribe
left a comment
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
Promisefail theIllegalImportcheck 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()))); |
| if (!drainComplete.await(DatabaseDescriptor.getGracefulDisconnectGracePeriod(), MILLISECONDS)) | ||
| throw new TimeoutException("Timed out waiting for drain to complete after graceful disconnect"); |
| if (connectedChannels.decrementAndGet() == 0) | ||
| { | ||
| timeoutTask.cancel(false); | ||
| runOnceAction.run(); |
| 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(); |
| AtomicBoolean actionStarted = new AtomicBoolean(false); | ||
| AtomicInteger connectedChannels = new AtomicInteger(channelGroup.size()); |
| 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
left a comment
There was a problem hiding this comment.
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"); |
There was a problem hiding this comment.
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()))); |
| } | ||
| }); | ||
| if (!drainComplete.await(DatabaseDescriptor.getGracefulDisconnectGracePeriod(), MILLISECONDS)) | ||
| throw new TimeoutException("Timed out waiting for drain to complete after graceful disconnect"); |
There was a problem hiding this comment.
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; |
|
@SiyaoIsHiding I need a java driver supporting cep 59. I am unable to get one, so further testing can't be done by me. |
|
@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. |
|
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. |
|
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 |
There was a problem hiding this comment.
I think you missed a semicolon at the end of this import.
There was a problem hiding this comment.
yup, I also skipped the import style check skip patching it now.
There was a problem hiding this comment.
Also wait some moment before reviewing the StorageService class, I am going to change the approach there.
bdeaf90 to
c0641da
Compare
|
added new logic for draining.
|
Cassandra Jira 21191