You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by "AshenScribe (via GitHub)" <gi...@apache.org> on 2026/07/22 03:37:19 UTC
[PR] cep 59 [cassandra]
AshenScribe opened a new pull request, #4953:
URL: https://github.com/apache/cassandra/pull/4953
[Cassandra Jira 21191](https://issues.apache.org/jira/browse/CASSANDRA-21191)
--
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
Re: [PR] CASSANDRA-21191: [CEP-59] Implementation of In-Band Connection Draining (Graceful Disconnect) [cassandra]
Posted by "SiyaoIsHiding (via GitHub)" <gi...@apache.org>.
SiyaoIsHiding commented on code in PR #4953:
URL: https://github.com/apache/cassandra/pull/4953#discussion_r4076988135
##########
src/java/org/apache/cassandra/transport/Server.java:
##########
@@ -126,15 +128,60 @@ private Server (Builder builder)
Schema.instance.registerListener(notifier);
}
+ public ChannelGroup getChannelsSubscribedToGracefulDisconnect()
+ {
+ return connectionTracker.groups.get(Event.Type.GRACEFUL_DISCONNECT);
+ }
+
public void stop()
{
stop(false);
}
public void stop(boolean force)
{
- if (isRunning.compareAndSet(true, false))
- close(force);
+ if (isRunning.compareAndSet(true, false))
+ {
+ if (!force && DatabaseDescriptor.getGracefulDisconnectEnabled())
+ gracefulDisconnect();
+ close(force);
+ }
+ }
+
+ private void gracefulDisconnect()
+ {
+ stopAcceptingNewConnections();
+
+ ChannelGroup channelGroup = getChannelsSubscribedToGracefulDisconnect();
+ if (channelGroup.isEmpty())
+ return;
+
+ channelGroup.forEach(channel ->
+ {
+ ClientMetrics.instance.connectionsDraining.set(channelGroup.size());
Review Comment:
This line should be before the for loop or the same set size will be run multiple times.
##########
test/unit/org/apache/cassandra/service/StorageServiceTest.java:
##########
@@ -201,6 +202,32 @@ public void testColumnIndexSizeInKiB()
}
}
+ @Test
+ public void testGracefulDisconnectGracePeriod()
+ {
+ StorageService storageService = StorageService.instance;
+ long originalGracePeriod = storageService.getGracefulDisconnectGracePeriod();
+ try
+ {
+ storageService.setGracefulDisconnectGracePeriod(3000);
+ Assertions.assertThat(3000).isEqualTo(storageService.getGracefulDisconnectGracePeriod());
+
+ Assertions.assertThatThrownBy(() -> storageService.setGracefulDisconnectGracePeriod(-1)).isInstanceOf(IllegalArgumentException.class);
+
+ Assertions.assertThat(3000).isEqualTo(storageService.getGracefulDisconnectGracePeriod());
+ }
+ finally
+ {
+ storageService.setGracefulDisconnectGracePeriod(originalGracePeriod);
+ }
+ }
+
+ @Test
+ public void testGracefulDisconnectEnabled()
+ {
+ Assertions.assertThat(StorageService.instance.getGracefulDisconnectEnabled()).isFalse();
Review Comment:
Kinda confused by this test. Why is it called "testGracefulDisconnectEnabled" but the assertion is `instance.getGracefulDisconnectEnabled()).isFalse();
}`? Is it supposed to test the default?
##########
doc/native_protocol_v5.spec:
##########
@@ -987,6 +998,8 @@ Table of Contents
consists of a [string] and an [inet], corresponding respectively to the
type of change ("NEW_NODE" or "REMOVED_NODE") followed by the address of
the new/removed node.
+
Review Comment:
remove the two lines
##########
src/java/org/apache/cassandra/service/NativeTransportService.java:
##########
@@ -163,6 +163,12 @@ Server getServer()
return server;
}
+ @VisibleForTesting
Review Comment:
Add
```
// for testing purpose
```
Will be helpful. Otherwise people would wonder why this line is here if it's not used in src in anyway.
##########
src/java/org/apache/cassandra/transport/Server.java:
##########
@@ -126,15 +128,60 @@ private Server (Builder builder)
Schema.instance.registerListener(notifier);
}
+ public ChannelGroup getChannelsSubscribedToGracefulDisconnect()
+ {
+ return connectionTracker.groups.get(Event.Type.GRACEFUL_DISCONNECT);
+ }
+
public void stop()
{
stop(false);
}
public void stop(boolean force)
{
- if (isRunning.compareAndSet(true, false))
- close(force);
+ if (isRunning.compareAndSet(true, false))
+ {
+ if (!force && DatabaseDescriptor.getGracefulDisconnectEnabled())
+ gracefulDisconnect();
+ close(force);
+ }
+ }
+
+ private void gracefulDisconnect()
+ {
+ stopAcceptingNewConnections();
+
+ ChannelGroup channelGroup = getChannelsSubscribedToGracefulDisconnect();
+ if (channelGroup.isEmpty())
+ return;
+
+ channelGroup.forEach(channel ->
+ {
+ ClientMetrics.instance.connectionsDraining.set(channelGroup.size());
+ channel.closeFuture().addListener(future -> ClientMetrics.instance.decrementConnectionsDraining());
+ }
+ );
Review Comment:
+1 makes sense to me
##########
src/java/org/apache/cassandra/transport/SimpleClient.java:
##########
@@ -322,8 +345,14 @@ public Message.Response execute(Message.Request request)
return execute(request, true);
}
+
public Message.Response execute(Message.Request request, boolean throwOnErrorResponse)
{
+ if (draining.get())
+ throw new RuntimeException("Connection is draining (GRACEFUL_DISCONNECT received)");
+
+ CompletableFuture<Void> requestCompletion = new CompletableFuture<>();
+ inFlight = requestCompletion;
Review Comment:
Both points sound correct to me
##########
src/java/org/apache/cassandra/transport/Server.java:
##########
@@ -126,15 +128,60 @@ private Server (Builder builder)
Schema.instance.registerListener(notifier);
}
+ public ChannelGroup getChannelsSubscribedToGracefulDisconnect()
+ {
+ return connectionTracker.groups.get(Event.Type.GRACEFUL_DISCONNECT);
+ }
+
public void stop()
{
stop(false);
}
public void stop(boolean force)
{
- if (isRunning.compareAndSet(true, false))
- close(force);
+ if (isRunning.compareAndSet(true, false))
+ {
+ if (!force && DatabaseDescriptor.getGracefulDisconnectEnabled())
+ gracefulDisconnect();
Review Comment:
+1. Pls refer to https://github.com/SiyaoIsHiding/cassandra/commit/dcbd99aff582d13827ef4d3030be04626c99ae3b#diff-0c9899065ce3b2c6d2782334f104ca56339af300fff9c8a00450ff31fc069343
On how to add another flag to stop throwing `OverloadedException`
##########
test/distributed/org/apache/cassandra/distributed/test/GracefulDisconnectTest.java:
##########
@@ -0,0 +1,488 @@
+/*
+ * 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 org.apache.cassandra.distributed.test;
+
+import java.io.IOException;
+import java.net.InetSocketAddress;
+import java.nio.charset.StandardCharsets;
+import java.util.Collections;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.TimeUnit;
+
+import org.awaitility.Awaitility;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import org.apache.cassandra.config.DatabaseDescriptor;
+import org.apache.cassandra.cql3.QueryOptions;
+import org.apache.cassandra.distributed.Cluster;
+import org.apache.cassandra.distributed.api.Feature;
+import org.apache.cassandra.metrics.ClientMetrics;
+import org.apache.cassandra.service.CassandraDaemon;
+import org.apache.cassandra.transport.Event;
+import org.apache.cassandra.transport.Message;
+import org.apache.cassandra.transport.ProtocolVersion;
+import org.apache.cassandra.transport.SimpleClient;
+import org.apache.cassandra.transport.messages.OptionsMessage;
+import org.apache.cassandra.transport.messages.QueryMessage;
+import org.apache.cassandra.transport.messages.ReadyMessage;
+import org.apache.cassandra.transport.messages.RegisterMessage;
+import org.apache.cassandra.transport.messages.ResultMessage;
+import org.apache.cassandra.transport.messages.StartupMessage;
+import org.apache.cassandra.transport.messages.SupportedMessage;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+public class GracefulDisconnectTest
+{
+
+ @BeforeClass
+ public static void setUp() throws IOException
+ {
+ DatabaseDescriptor.daemonInitialization();
+ }
+
+ public Cluster buildCluster(int nodeCount, boolean gracefulDisconnectEnabled) throws IOException
+ {
+ return Cluster
+ .build(nodeCount)
+ .withConfig(config ->
+ config
+ .with(Feature.NATIVE_PROTOCOL, Feature.GOSSIP)
+ .set("graceful_disconnect_enabled", gracefulDisconnectEnabled))
+ .start();
+ }
+
+ @Test
+ public void testGracefulDisconnectAdvertisedWhenEnabled() throws IOException
+ {
+ try (Cluster cluster = buildCluster(1, true))
+ {
+ InetSocketAddress nativeAddr = cluster.get(1).config().broadcastAddress();
+ try (SimpleClient client = SimpleClient.builder(nativeAddr.getHostString(), 9042).build())
+ {
+ client.connect(false);
+ Message.Response response = client.execute(new OptionsMessage());
+ SupportedMessage supported = (SupportedMessage) response;
+
+ assertThat(supported.supported.containsKey(StartupMessage.GRACEFUL_DISCONNECT))
+ .as("GRACEFUL_DISCONNECT should be advertised in SUPPORTED when enabled")
+ .isTrue();
+ }
+ }
+ }
+
+ @Test
+ public void testGracefulDisconnectDoesNotAdvertisedWhenNotEnabled() throws IOException
+ {
+ try (Cluster cluster = buildCluster(1, false))
+ {
+ InetSocketAddress nativeAddr = cluster.get(1).config().broadcastAddress();
+ try (SimpleClient client = SimpleClient.builder(nativeAddr.getHostString(), 9042).build())
+ {
+ client.connect(false);
+ SupportedMessage supported = (SupportedMessage) client.execute(new OptionsMessage());
+
+ assertThat(supported.supported.containsKey(StartupMessage.GRACEFUL_DISCONNECT))
+ .as("GRACEFUL_DISCONNECT should NOT be advertised in SUPPORTED when disabled")
+ .isFalse();
+ }
+ }
+ }
+
+ @Test
+ public void testSubscriptionViaREGISTER() throws IOException
+ {
+ try (Cluster cluster = buildCluster(1, true))
+ {
+ InetSocketAddress nativeAddr = cluster.get(1).config().broadcastAddress();
+ try (SimpleClient client = SimpleClient.builder(nativeAddr.getHostString(), 9042)
+ .protocolVersion(ProtocolVersion.V5)
+ .build())
+ {
+ client.connect(false);
+ Message.Response response = client.execute(
+ new RegisterMessage(Collections.singletonList(Event.Type.GRACEFUL_DISCONNECT)));
+
+ assertThat(response).isInstanceOf(ReadyMessage.class);
+
+ int subscribedCount = cluster.get(1).callOnInstance(() ->
+ CassandraDaemon.getInstanceForTesting()
+ .nativeTransportService()
+ .getChannelsSubscribedToGracefulDisconnectCount());
+
+ assertThat(subscribedCount)
+ .as("One channel should be subscribed to GRACEFUL_DISCONNECT")
+ .isEqualTo(1);
+ }
+ }
+ }
+
+ @Test
+ public void testRegisterGracefulDisconnectAcceptedOnV4() throws IOException
+ {
+ try (Cluster cluster = buildCluster(1, true))
+ {
+ InetSocketAddress nativeAddr = cluster.get(1).config().broadcastAddress();
+ try (SimpleClient client = SimpleClient.builder(nativeAddr.getHostString(), 9042).protocolVersion(ProtocolVersion.V4).build())
+ {
+ client.connect(false);
+ client.execute(new RegisterMessage(Collections.singletonList(Event.Type.GRACEFUL_DISCONNECT)));
+ int subscribedCount = cluster.get(1).callOnInstance(() ->
+ CassandraDaemon.getInstanceForTesting()
+ .nativeTransportService()
+ .getChannelsSubscribedToGracefulDisconnectCount());
+ assertThat(subscribedCount)
+ .as("V4 client should be able to subscribe")
+ .isEqualTo(1);
+ }
+ }
+ }
+
+ @Test
+ public void testNonSubscribedClientDoesNotReceiveEvent() throws IOException
+ {
+ try (Cluster cluster = buildCluster(1, true))
+ {
+ InetSocketAddress nativeAddr = cluster.get(1).config().broadcastAddress();
+ try (SimpleClient client = SimpleClient.builder(nativeAddr.getHostString(), 9042)
+ .protocolVersion(ProtocolVersion.V4)
+ .build())
+ {
+ client.connect(false);
+ int subscribedCount = cluster.get(1).callOnInstance(() ->
+ CassandraDaemon.getInstanceForTesting()
+ .nativeTransportService()
+ .getChannelsSubscribedToGracefulDisconnectCount());
+ assertThat(subscribedCount).isEqualTo(0);
+ }
+ }
+ }
+
+ @Test
+ public void testServerStopsAcceptingNewConnectionsOnDrain() throws IOException
+ {
+ try (Cluster cluster = buildCluster(1, true))
+ {
+ InetSocketAddress nativeAddr = cluster.get(1).config().broadcastAddress();
+ try (SimpleClient client = SimpleClient.builder(nativeAddr.getHostString(), 9042).build())
+ {
+ client.connect(false);
+ cluster.get(1).nodetool("drain");
+
+ assertThatThrownBy(() -> {
+ SimpleClient newClient = SimpleClient.builder(nativeAddr.getHostString(), 9042).build();
+ newClient.connect(false);
+ }).as("Server should reject new connections after stopAcceptingNewConnections")
+ .isNotNull();
+ cluster.get(1).shutdown();
+ }
+ }
+ }
+
+ @Test
+ public void testNoEventEmittedWhenDisabled() throws IOException
+ {
+ try (Cluster cluster = buildCluster(1, false))
+ {
+ InetSocketAddress nativeAddr = cluster.get(1).config().broadcastAddress();
+ try (SimpleClient client = SimpleClient.builder(nativeAddr.getHostString(), 9042).build())
+ {
+ client.connect(false);
+ client.execute(new RegisterMessage(Collections.singletonList(Event.Type.GRACEFUL_DISCONNECT)));
+
+ int subscribedCount = cluster.get(1).callOnInstance(() ->
+ CassandraDaemon.getInstanceForTesting()
+ .nativeTransportService()
+ .getChannelsSubscribedToGracefulDisconnectCount());
+
+ assertThat(subscribedCount).isEqualTo(0);
+
+ boolean disabled = cluster.get(1).callOnInstance(() -> !DatabaseDescriptor.getGracefulDisconnectEnabled());
+ assertThat(disabled).isTrue();
+ }
+ }
+ }
+
+ @Test
+ public void testDrainProceedsImmediatelyWithNoSubscribedConnections() throws IOException
+ {
+ try (Cluster cluster = buildCluster(1, true))
+ {
+ InetSocketAddress nativeAddr = cluster.get(1).config().broadcastAddress();
+ try (SimpleClient client = SimpleClient.builder(nativeAddr.getHostString(), 9042)
+ .protocolVersion(ProtocolVersion.V4)
+ .build())
+ {
+ client.connect(false);
+ cluster.get(1).nodetoolResult("drain").asserts().success();
+ cluster.get(1).shutdown();
+ }
+ }
+ }
+
+ @Test
+ public void testMultipleConnectionsCanSubscribe() throws IOException
+ {
+ try (Cluster cluster = buildCluster(1, true))
+ {
+ InetSocketAddress nativeAddr = cluster.get(1).config().broadcastAddress();
+ try (SimpleClient client1 = SimpleClient.builder(nativeAddr.getHostString(), 9042)
+ .protocolVersion(ProtocolVersion.V5)
+ .build();
+ SimpleClient client2 = SimpleClient.builder(nativeAddr.getHostString(), 9042)
+ .protocolVersion(ProtocolVersion.V5)
+ .build())
+ {
+ client1.connect(false);
+ client2.connect(false);
+
+ client1.execute(new RegisterMessage(Collections.singletonList(Event.Type.GRACEFUL_DISCONNECT)));
+ client2.execute(new RegisterMessage(Collections.singletonList(Event.Type.GRACEFUL_DISCONNECT)));
+
+ int subscribedCount = cluster.get(1).callOnInstance(() ->
+ CassandraDaemon.getInstanceForTesting()
+ .nativeTransportService()
+ .getChannelsSubscribedToGracefulDisconnectCount());
+
+ assertThat(subscribedCount).isEqualTo(2);
+ }
+ }
+ }
+
+ @Test
+ public void testInFlightQueryCompletesDuringDrain() throws Exception
+ {
+ try (Cluster cluster = buildCluster(1, true))
+ {
+ cluster.schemaChange("CREATE KEYSPACE ks WITH replication = {'class': 'SimpleStrategy', 'replication_factor': 1}");
+ cluster.schemaChange("CREATE TABLE ks.tbl (id int PRIMARY KEY, val text)");
+ cluster.get(1).executeInternal("INSERT INTO ks.tbl (id, val) VALUES (1, 'test_val')");
+
+ InetSocketAddress nativeAddr = cluster.get(1).config().broadcastAddress();
+ try (SimpleClient client = SimpleClient.builder(nativeAddr.getHostString(), 9042)
+ .protocolVersion(ProtocolVersion.V5)
+ .build())
+ {
+ client.connect(false);
+ client.execute(new RegisterMessage(Collections.singletonList(Event.Type.GRACEFUL_DISCONNECT)));
+
+ CompletableFuture<Void> drainFuture = CompletableFuture.runAsync(() -> {
+ cluster.get(1).nodetool("drain");
+ });
+
+ Message.Response response = client.execute(new org.apache.cassandra.transport.messages.QueryMessage(
Review Comment:
Please change all those full path imports into a proper `import` statement and only the class name in-line, just like human-written code. This applies to the whole PR.
##########
src/java/org/apache/cassandra/transport/Server.java:
##########
@@ -126,15 +128,60 @@ private Server (Builder builder)
Schema.instance.registerListener(notifier);
}
+ public ChannelGroup getChannelsSubscribedToGracefulDisconnect()
+ {
+ return connectionTracker.groups.get(Event.Type.GRACEFUL_DISCONNECT);
+ }
+
public void stop()
{
stop(false);
}
public void stop(boolean force)
{
- if (isRunning.compareAndSet(true, false))
- close(force);
+ if (isRunning.compareAndSet(true, false))
+ {
+ if (!force && DatabaseDescriptor.getGracefulDisconnectEnabled())
+ gracefulDisconnect();
+ close(force);
+ }
+ }
+
+ private void gracefulDisconnect()
+ {
+ stopAcceptingNewConnections();
+
+ ChannelGroup channelGroup = getChannelsSubscribedToGracefulDisconnect();
+ if (channelGroup.isEmpty())
+ return;
+
+ channelGroup.forEach(channel ->
+ {
+ ClientMetrics.instance.connectionsDraining.set(channelGroup.size());
+ channel.closeFuture().addListener(future -> ClientMetrics.instance.decrementConnectionsDraining());
+ }
+ );
Review Comment:
makes sense to me
##########
src/java/org/apache/cassandra/service/NativeTransportService.java:
##########
@@ -163,6 +163,12 @@ Server getServer()
return server;
}
+ @VisibleForTesting
Review Comment:
It's already public, no need for `@VisibleForTesting`, which is used to escalate a private or protected method so tests can access it.
##########
src/java/org/apache/cassandra/transport/Server.java:
##########
@@ -157,6 +164,16 @@ public synchronized void start()
isRunning.set(true);
}
+ public void stopAcceptingNewConnections()
+ {
+ 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();
Review Comment:
Wow excellent point. I think updating `ConnectionTracker.countConnectedClients()` is the move here.
##########
src/java/org/apache/cassandra/config/Config.java:
##########
@@ -143,6 +143,10 @@ public static Set<String> splitCommaDelimited(String src)
/** Triggers automatic allocation of tokens if set, based on the provided replica count for a datacenter */
public Integer allocate_tokens_for_local_replication_factor = null;
+ public boolean graceful_disconnect_enabled = false;
+
+ public volatile DurationSpec.LongMillisecondsBound graceful_disconnect_grace_period = new DurationSpec.LongMillisecondsBound(5000);
Review Comment:
It's commented in this line because this is where yaml config is loaded. Yaml config isn't loaded through `DatabaseDescriptor.setGracefulDisconnectGracePeriod`, so it's never validated.
The changes needed is at `DatabaseDescriptor.applySimpleConfig`, you can add grace period validation there.
##########
test/distributed/org/apache/cassandra/distributed/test/GracefulDisconnectTest.java:
##########
@@ -0,0 +1,488 @@
+/*
+ * 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 org.apache.cassandra.distributed.test;
+
+import java.io.IOException;
+import java.net.InetSocketAddress;
+import java.nio.charset.StandardCharsets;
+import java.util.Collections;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.TimeUnit;
+
+import org.awaitility.Awaitility;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import org.apache.cassandra.config.DatabaseDescriptor;
+import org.apache.cassandra.cql3.QueryOptions;
+import org.apache.cassandra.distributed.Cluster;
+import org.apache.cassandra.distributed.api.Feature;
+import org.apache.cassandra.metrics.ClientMetrics;
+import org.apache.cassandra.service.CassandraDaemon;
+import org.apache.cassandra.transport.Event;
+import org.apache.cassandra.transport.Message;
+import org.apache.cassandra.transport.ProtocolVersion;
+import org.apache.cassandra.transport.SimpleClient;
+import org.apache.cassandra.transport.messages.OptionsMessage;
+import org.apache.cassandra.transport.messages.QueryMessage;
+import org.apache.cassandra.transport.messages.ReadyMessage;
+import org.apache.cassandra.transport.messages.RegisterMessage;
+import org.apache.cassandra.transport.messages.ResultMessage;
+import org.apache.cassandra.transport.messages.StartupMessage;
+import org.apache.cassandra.transport.messages.SupportedMessage;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+public class GracefulDisconnectTest
+{
+
+ @BeforeClass
+ public static void setUp() throws IOException
+ {
+ DatabaseDescriptor.daemonInitialization();
+ }
+
+ public Cluster buildCluster(int nodeCount, boolean gracefulDisconnectEnabled) throws IOException
+ {
+ return Cluster
+ .build(nodeCount)
+ .withConfig(config ->
+ config
+ .with(Feature.NATIVE_PROTOCOL, Feature.GOSSIP)
+ .set("graceful_disconnect_enabled", gracefulDisconnectEnabled))
+ .start();
+ }
+
+ @Test
+ public void testGracefulDisconnectAdvertisedWhenEnabled() throws IOException
+ {
+ try (Cluster cluster = buildCluster(1, true))
+ {
+ InetSocketAddress nativeAddr = cluster.get(1).config().broadcastAddress();
+ try (SimpleClient client = SimpleClient.builder(nativeAddr.getHostString(), 9042).build())
+ {
+ client.connect(false);
+ Message.Response response = client.execute(new OptionsMessage());
+ SupportedMessage supported = (SupportedMessage) response;
+
+ assertThat(supported.supported.containsKey(StartupMessage.GRACEFUL_DISCONNECT))
+ .as("GRACEFUL_DISCONNECT should be advertised in SUPPORTED when enabled")
+ .isTrue();
+ }
+ }
+ }
+
+ @Test
+ public void testGracefulDisconnectDoesNotAdvertisedWhenNotEnabled() throws IOException
+ {
+ try (Cluster cluster = buildCluster(1, false))
+ {
+ InetSocketAddress nativeAddr = cluster.get(1).config().broadcastAddress();
+ try (SimpleClient client = SimpleClient.builder(nativeAddr.getHostString(), 9042).build())
+ {
+ client.connect(false);
+ SupportedMessage supported = (SupportedMessage) client.execute(new OptionsMessage());
+
+ assertThat(supported.supported.containsKey(StartupMessage.GRACEFUL_DISCONNECT))
+ .as("GRACEFUL_DISCONNECT should NOT be advertised in SUPPORTED when disabled")
+ .isFalse();
+ }
+ }
+ }
+
+ @Test
+ public void testSubscriptionViaREGISTER() throws IOException
+ {
+ try (Cluster cluster = buildCluster(1, true))
+ {
+ InetSocketAddress nativeAddr = cluster.get(1).config().broadcastAddress();
+ try (SimpleClient client = SimpleClient.builder(nativeAddr.getHostString(), 9042)
+ .protocolVersion(ProtocolVersion.V5)
+ .build())
+ {
+ client.connect(false);
+ Message.Response response = client.execute(
+ new RegisterMessage(Collections.singletonList(Event.Type.GRACEFUL_DISCONNECT)));
+
+ assertThat(response).isInstanceOf(ReadyMessage.class);
+
+ int subscribedCount = cluster.get(1).callOnInstance(() ->
+ CassandraDaemon.getInstanceForTesting()
+ .nativeTransportService()
+ .getChannelsSubscribedToGracefulDisconnectCount());
+
+ assertThat(subscribedCount)
+ .as("One channel should be subscribed to GRACEFUL_DISCONNECT")
+ .isEqualTo(1);
+ }
+ }
+ }
+
+ @Test
+ public void testRegisterGracefulDisconnectAcceptedOnV4() throws IOException
+ {
+ try (Cluster cluster = buildCluster(1, true))
+ {
+ InetSocketAddress nativeAddr = cluster.get(1).config().broadcastAddress();
+ try (SimpleClient client = SimpleClient.builder(nativeAddr.getHostString(), 9042).protocolVersion(ProtocolVersion.V4).build())
+ {
+ client.connect(false);
+ client.execute(new RegisterMessage(Collections.singletonList(Event.Type.GRACEFUL_DISCONNECT)));
+ int subscribedCount = cluster.get(1).callOnInstance(() ->
+ CassandraDaemon.getInstanceForTesting()
+ .nativeTransportService()
+ .getChannelsSubscribedToGracefulDisconnectCount());
+ assertThat(subscribedCount)
+ .as("V4 client should be able to subscribe")
+ .isEqualTo(1);
+ }
+ }
+ }
+
+ @Test
+ public void testNonSubscribedClientDoesNotReceiveEvent() throws IOException
+ {
+ try (Cluster cluster = buildCluster(1, true))
+ {
+ InetSocketAddress nativeAddr = cluster.get(1).config().broadcastAddress();
+ try (SimpleClient client = SimpleClient.builder(nativeAddr.getHostString(), 9042)
+ .protocolVersion(ProtocolVersion.V4)
+ .build())
+ {
+ client.connect(false);
+ int subscribedCount = cluster.get(1).callOnInstance(() ->
+ CassandraDaemon.getInstanceForTesting()
+ .nativeTransportService()
+ .getChannelsSubscribedToGracefulDisconnectCount());
+ assertThat(subscribedCount).isEqualTo(0);
+ }
+ }
+ }
+
+ @Test
+ public void testServerStopsAcceptingNewConnectionsOnDrain() throws IOException
+ {
+ try (Cluster cluster = buildCluster(1, true))
+ {
+ InetSocketAddress nativeAddr = cluster.get(1).config().broadcastAddress();
+ try (SimpleClient client = SimpleClient.builder(nativeAddr.getHostString(), 9042).build())
+ {
+ client.connect(false);
+ cluster.get(1).nodetool("drain");
+
+ assertThatThrownBy(() -> {
+ SimpleClient newClient = SimpleClient.builder(nativeAddr.getHostString(), 9042).build();
+ newClient.connect(false);
+ }).as("Server should reject new connections after stopAcceptingNewConnections")
+ .isNotNull();
+ cluster.get(1).shutdown();
+ }
+ }
+ }
+
+ @Test
+ public void testNoEventEmittedWhenDisabled() throws IOException
+ {
+ try (Cluster cluster = buildCluster(1, false))
+ {
+ InetSocketAddress nativeAddr = cluster.get(1).config().broadcastAddress();
+ try (SimpleClient client = SimpleClient.builder(nativeAddr.getHostString(), 9042).build())
+ {
+ client.connect(false);
+ client.execute(new RegisterMessage(Collections.singletonList(Event.Type.GRACEFUL_DISCONNECT)));
+
+ int subscribedCount = cluster.get(1).callOnInstance(() ->
+ CassandraDaemon.getInstanceForTesting()
+ .nativeTransportService()
+ .getChannelsSubscribedToGracefulDisconnectCount());
+
+ assertThat(subscribedCount).isEqualTo(0);
+
+ boolean disabled = cluster.get(1).callOnInstance(() -> !DatabaseDescriptor.getGracefulDisconnectEnabled());
+ assertThat(disabled).isTrue();
+ }
+ }
+ }
+
+ @Test
+ public void testDrainProceedsImmediatelyWithNoSubscribedConnections() throws IOException
+ {
+ try (Cluster cluster = buildCluster(1, true))
+ {
+ InetSocketAddress nativeAddr = cluster.get(1).config().broadcastAddress();
+ try (SimpleClient client = SimpleClient.builder(nativeAddr.getHostString(), 9042)
+ .protocolVersion(ProtocolVersion.V4)
+ .build())
+ {
+ client.connect(false);
+ cluster.get(1).nodetoolResult("drain").asserts().success();
+ cluster.get(1).shutdown();
+ }
+ }
+ }
+
+ @Test
+ public void testMultipleConnectionsCanSubscribe() throws IOException
+ {
+ try (Cluster cluster = buildCluster(1, true))
+ {
+ InetSocketAddress nativeAddr = cluster.get(1).config().broadcastAddress();
+ try (SimpleClient client1 = SimpleClient.builder(nativeAddr.getHostString(), 9042)
+ .protocolVersion(ProtocolVersion.V5)
+ .build();
+ SimpleClient client2 = SimpleClient.builder(nativeAddr.getHostString(), 9042)
+ .protocolVersion(ProtocolVersion.V5)
+ .build())
+ {
+ client1.connect(false);
+ client2.connect(false);
+
+ client1.execute(new RegisterMessage(Collections.singletonList(Event.Type.GRACEFUL_DISCONNECT)));
+ client2.execute(new RegisterMessage(Collections.singletonList(Event.Type.GRACEFUL_DISCONNECT)));
+
+ int subscribedCount = cluster.get(1).callOnInstance(() ->
+ CassandraDaemon.getInstanceForTesting()
+ .nativeTransportService()
+ .getChannelsSubscribedToGracefulDisconnectCount());
+
+ assertThat(subscribedCount).isEqualTo(2);
+ }
+ }
+ }
+
+ @Test
+ public void testInFlightQueryCompletesDuringDrain() throws Exception
+ {
+ try (Cluster cluster = buildCluster(1, true))
+ {
+ cluster.schemaChange("CREATE KEYSPACE ks WITH replication = {'class': 'SimpleStrategy', 'replication_factor': 1}");
+ cluster.schemaChange("CREATE TABLE ks.tbl (id int PRIMARY KEY, val text)");
+ cluster.get(1).executeInternal("INSERT INTO ks.tbl (id, val) VALUES (1, 'test_val')");
+
+ InetSocketAddress nativeAddr = cluster.get(1).config().broadcastAddress();
+ try (SimpleClient client = SimpleClient.builder(nativeAddr.getHostString(), 9042)
+ .protocolVersion(ProtocolVersion.V5)
+ .build())
+ {
+ client.connect(false);
+ client.execute(new RegisterMessage(Collections.singletonList(Event.Type.GRACEFUL_DISCONNECT)));
+
+ CompletableFuture<Void> drainFuture = CompletableFuture.runAsync(() -> {
+ cluster.get(1).nodetool("drain");
+ });
+
+ Message.Response response = client.execute(new org.apache.cassandra.transport.messages.QueryMessage(
Review Comment:
+1, refer to the below tests on how to use Awaitability
--
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
Re: [PR] CASSANDRA-21191: [CEP-59] Implementation of In-Band Connection Draining (Graceful Disconnect) [cassandra]
Posted by "AshenScribe (via GitHub)" <gi...@apache.org>.
AshenScribe commented on PR #4953:
URL: https://github.com/apache/cassandra/pull/4953#issuecomment-5356290306
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](https://github.com/apache/cassandra-java-driver/pull/2091) and [native-protocol](https://github.com/datastax/native-protocol/pull/61) and this PR. Current test include draining with `graceful_disconnect_enabled` set to true and false and used cqlsh to test legacy behaviour.
--
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
Re: [PR] CASSANDRA-21191: [CEP-59] Implementation of In-Band Connection Draining (Graceful Disconnect) [cassandra]
Posted by "AshenScribe (via GitHub)" <gi...@apache.org>.
AshenScribe commented on PR #4953:
URL: https://github.com/apache/cassandra/pull/4953#issuecomment-5407240261
@SiyaoIsHiding I inspected the files to find out call stack. I made this call stack graph.
<img width="6557" height="8192" alt="Call Stack" src="https://github.com/user-attachments/assets/d8195c6a-1013-42ed-a02d-585b41336317" />
Let me know if this aligns with your understand of drain flow.
So here only drain change is in Transport layer.
--
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
Re: [PR] CASSANDRA-21191: [CEP-59] Implementation of In-Band Connection Draining (Graceful Disconnect) [cassandra]
Posted by "AshenScribe (via GitHub)" <gi...@apache.org>.
AshenScribe commented on PR #4953:
URL: https://github.com/apache/cassandra/pull/4953#issuecomment-5521408466
the most recent force push was for in complete responses to changes requested..
--
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
Re: [PR] CASSANDRA-21191: [CEP-59] Implementation of In-Band Connection Draining (Graceful Disconnect) [cassandra]
Posted by "AshenScribe (via GitHub)" <gi...@apache.org>.
AshenScribe commented on code in PR #4953:
URL: https://github.com/apache/cassandra/pull/4953#discussion_r3921161594
##########
src/java/org/apache/cassandra/service/NativeTransportService.java:
##########
@@ -163,6 +164,11 @@ Server getServer()
return server;
}
+ public ChannelGroup getChannelsSubscribedToGracefulDisconnect()
Review Comment:
Ig we must use @VisibleForTesting annotation
--
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
Re: [PR] CASSANDRA-21191: [CEP-59] Implementation of In-Band Connection Draining (Graceful Disconnect) [cassandra]
Posted by "SiyaoIsHiding (via GitHub)" <gi...@apache.org>.
SiyaoIsHiding commented on code in PR #4953:
URL: https://github.com/apache/cassandra/pull/4953#discussion_r4341749461
##########
doc/modules/cassandra/pages/managing/operating/graceful_disconnect.adoc:
##########
@@ -0,0 +1,91 @@
+= Graceful Disconnect — In-Band Connection Draining for Cassandra Node Shutdown
Review Comment:
> "What does the ticket lacks that a doc file is needed? "
This
<p style="margin: 10px 0px 0px; padding: 0px;"><span style="color: rgb(182, 194, 207);">Such server shutdown behavior applies as follows.</span></p><div class="table-wrap" style="margin: 10px 0px 0px; padding: 0px; overflow-x: auto;">
nodetool disablebinary | yes
nodetool disablebinary -f ("Use -f to interrupt client requests that have already started") | no
nodetool drain | yes
nodetool stopdaemon | yes
nodetool decommission | yes
nodetool decommission -f | yes
JVM shutdown hook/ SIGTERM/ systemctl stop | yes
ccm node1 stop (SIGTERM) | yes
SIGKILL | no
</div><p style="margin: 10px 0px 0px; padding: 0px;"><br style="color: rgb(182, 194, 207); font-family: -apple-system, BlinkMacSystemFont, "Segoe UI", Roboto, Oxygen, Ubuntu, "Fira Sans", "Droid Sans", "Helvetica Neue", sans-serif; font-size: 14px; font-style: normal; font-variant-ligatures: normal; font-variant-caps: normal; font-weight: 400; letter-spacing: normal; orphans: 2; text-align: left; text-indent: 0px; text-transform: none; widows: 2; word-spacing: 0px; -webkit-text-stroke-width: 0px; white-space: normal; background-color: rgb(29, 33, 37); text-decoration-thickness: initial; text-decoration-style: initial; text-decoration-color: initial;">
And the 2 configs and 2 metrics to interpret whether graceful disconnect is working well. And what CQL protocol versions it supports. Driver compatibility matrix (we got asked a lot about compatibility matrix even tho they can technically search for the relevant the JIRA ticket and PR status).
A page in the manual also allows users to discover this by default disabled feature from browsing the manual, not only from reading `cassandra.yaml` comments.
--
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
Re: [PR] CASSANDRA-21191: [CEP-59] Implementation of In-Band Connection Draining (Graceful Disconnect) [cassandra]
Posted by "AshenScribe (via GitHub)" <gi...@apache.org>.
AshenScribe commented on code in PR #4953:
URL: https://github.com/apache/cassandra/pull/4953#discussion_r3815763819
##########
src/java/org/apache/cassandra/service/StorageService.java:
##########
@@ -40,13 +40,17 @@
import java.util.UUID;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.CopyOnWriteArrayList;
+import org.apache.cassandra.utils.concurrent.CountDownLatch
Review Comment:
Also wait some moment before reviewing the StorageService class, I am going to change the approach there.
--
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
Re: [PR] CASSANDRA-21191: [CEP-59] Implementation of In-Band Connection Draining (Graceful Disconnect) [cassandra]
Posted by "SiyaoIsHiding (via GitHub)" <gi...@apache.org>.
SiyaoIsHiding commented on code in PR #4953:
URL: https://github.com/apache/cassandra/pull/4953#discussion_r4150660870
##########
src/java/org/apache/cassandra/service/NativeTransportService.java:
##########
@@ -163,6 +163,12 @@ Server getServer()
return server;
}
+ @VisibleForTesting
Review Comment:
Agree above ^
--
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
Re: [PR] CASSANDRA-21191: [CEP-59] Implementation of In-Band Connection Draining (Graceful Disconnect) [cassandra]
Posted by "SiyaoIsHiding (via GitHub)" <gi...@apache.org>.
SiyaoIsHiding commented on code in PR #4953:
URL: https://github.com/apache/cassandra/pull/4953#discussion_r3936770086
##########
src/java/org/apache/cassandra/transport/Server.java:
##########
@@ -330,6 +395,8 @@ public boolean isRunning()
public void register(Event.Type type, Channel ch)
{
+ if (type == Event.Type.GRACEFUL_DISCONNECT && !DatabaseDescriptor.getGracefulDisconnectEnabled())
+ return;
Review Comment:
When a client sends a REGISTER of GRACEFUL_DISCONNECT when the server has graceful disconnect disabled, either ignoring or returning an error message makes sense to me.
Returning error message gives clear signal to the client as it should be. But it seems like the server previously just no-op on events it does not recognize. So, either is OK to me.
--
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
Re: [PR] CASSANDRA-21191: [CEP-59] Implementation of In-Band Connection Draining (Graceful Disconnect) [cassandra]
Posted by "SiyaoIsHiding (via GitHub)" <gi...@apache.org>.
SiyaoIsHiding commented on code in PR #4953:
URL: https://github.com/apache/cassandra/pull/4953#discussion_r3856791124
##########
src/java/org/apache/cassandra/service/NativeTransportService.java:
##########
@@ -163,6 +164,11 @@ Server getServer()
return server;
}
+ public ChannelGroup getChannelsSubscribedToGracefulDisconnect()
Review Comment:
This is only called in tests but not in source so should be removed
--
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
Re: [PR] CASSANDRA-21191: [CEP-59] Implementation of In-Band Connection Draining (Graceful Disconnect) [cassandra]
Posted by "AshenScribe (via GitHub)" <gi...@apache.org>.
AshenScribe commented on PR #4953:
URL: https://github.com/apache/cassandra/pull/4953#issuecomment-5408088981
I have added necessary test cases in the codebase.
Now starting manual testing. using this branch for server side, for [Java Driver](https://github.com/apache/cassandra-java-driver/pull/2091) and [native-protocol](https://github.com/datastax/native-protocol/pull/61) and stable [java driver](https://github.com/apache/cassandra-java-driver/tree/4.x) and stable [native-protocol](https://github.com/datastax/native-protocol) as un supported client.
To avoid redundant test cases across topologies (single vs multi node) and scale(flag true/false and driver supporting/non supportign), the 16 theoretical permutations cut down to 4 scenarios (and a mixed-driver scenario).
Needed Tests
Supporting Driver + Feature Enabled: Validates event registration, clean in-flight request draining, and clean socket closure (ForcedDisconnects = 0).
Supporting Driver + Feature Disabled: Validates that when disabled, capabilities are not advertised and the feature safely stays dormant.
Legacy Driver + Feature Enabled (Backward Compatibility): Validates that non-supporting drivers receive no events and fall back safely to standard TCP teardown without errors.
Uncooperative Supporting Driver (Timeout/Force-Close): Validates that if a driver ignores the event, the server honors graceful_disconnect_grace_period, forcefully severs the socket, and increments ForcedDisconnects.
Mixed-Fleet (Supporting + Legacy Clients Simultaneously Connected): Validates that supporting clients drain gracefully while legacy clients coexist on the same node without disruption and close on final server teardown.
Redundant test cases
Multi-Node vs. Single-Node Topologies: Graceful disconnect is a node-local Netty transport feature; cluster topology size does not alter connection teardown behavior.
Multi-Client Scaling Variations: Scaling from 1 client to 100 clients uses the exact same Netty ChannelGroup and atomic counters, making multi-client variations redundant once single-client and concurrent-client behaviors are verified in the codebase itself.
--
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
Re: [PR] CASSANDRA-21191: [CEP-59] Implementation of In-Band Connection Draining (Graceful Disconnect) [cassandra]
Posted by "SiyaoIsHiding (via GitHub)" <gi...@apache.org>.
SiyaoIsHiding commented on PR #4953:
URL: https://github.com/apache/cassandra/pull/4953#issuecomment-5415218903
The chart looks generally correct to me!
Re: `GracefulDisconnectLifecycle`, I think there might be a simplier and a cleaner way than a finite state machine. Now the only logic starts in either `NativeTransportService.stop(bool)` or `Server.stop()` but not in `StorageService`, the states should be simplier to manage. I'm still exploring cleaner ways to do that, you can try that out, too.
Re: the four testing scenarios you listed, you should check client side logs and see whether they are expected, too, apart from the server side logs.
Re: single node v.s. multi node, I personally prefer testing against multi node cluster because the primary purpose of graceful disconnect is that during rolling restarts, i.e. one node out of several is temporary down, the client application should be able to send the queries to other nodes so the client app can continue running, i.e. every request can get a response in time. If you use only single node and it's temporarily down, even if you have graceful disconnect, the client app will need to stop anyway because there just isn't any node that can give responses, most likely throwing AllNodesFailedException.
So I'd suggest you test against multi node cluster too.
--
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
Re: [PR] CASSANDRA-21191: [CEP-59] Implementation of In-Band Connection Draining (Graceful Disconnect) [cassandra]
Posted by "AshenScribe (via GitHub)" <gi...@apache.org>.
AshenScribe commented on PR #4953:
URL: https://github.com/apache/cassandra/pull/4953#issuecomment-5337021383
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.
--
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
Re: [PR] CASSANDRA-21191: [CEP-59] Implementation of In-Band Connection Draining (Graceful Disconnect) [cassandra]
Posted by "SiyaoIsHiding (via GitHub)" <gi...@apache.org>.
SiyaoIsHiding commented on PR #4953:
URL: https://github.com/apache/cassandra/pull/4953#issuecomment-5594863009
Please refer to: https://github.com/SiyaoIsHiding/cassandra/commit/dcbd99aff582d13827ef4d3030be04626c99ae3b
For end-to-end multi-node dtests and metric tests.
They are partially vibe-coded based on old commits so you will need to clean up and adapt to your current branch's implementation.
--
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
Re: [PR] CASSANDRA-21191: [CEP-59] Implementation of In-Band Connection Draining (Graceful Disconnect) [cassandra]
Posted by "AshenScribe (via GitHub)" <gi...@apache.org>.
AshenScribe commented on PR #4953:
URL: https://github.com/apache/cassandra/pull/4953#issuecomment-5415460820
If I understand correctly:
grace_disconnect is a connection-local event, so the server behaves identically whether it's a single node or a multi-node cluster.
once the server emits GRACEFUL_DISCONNECT, routing subsequent requests to other healthy nodes in topology is entirely the driver's responsibility.
The FSM introduced traverses on two possible paths WAITING_FOR_CLIENTS → COMPLETE when all clients cooperate or WAITING_FOR_CLIENTS → FORCE_CLOSING not all cooperate, so not that difficult of a path. I'll keep looking for better logic though.
--
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
Re: [PR] CASSANDRA-21191: [CEP-59] Implementation of In-Band Connection Draining (Graceful Disconnect) [cassandra]
Posted by "absurdfarce (via GitHub)" <gi...@apache.org>.
absurdfarce commented on code in PR #4953:
URL: https://github.com/apache/cassandra/pull/4953#discussion_r3918692745
##########
src/java/org/apache/cassandra/transport/Event.java:
##########
@@ -36,7 +36,8 @@ public enum Type
TOPOLOGY_CHANGE(ProtocolVersion.V3),
STATUS_CHANGE(ProtocolVersion.V3),
SCHEMA_CHANGE(ProtocolVersion.V3),
- TRACE_COMPLETE(ProtocolVersion.V4);
+ TRACE_COMPLETE(ProtocolVersion.V4),
+ GRACEFUL_DISCONNECT(ProtocolVersion.V5);
Review Comment:
There's no logical connection between the protocol version bound here and support for graceful disconnect; in theory most (all?) protocol versions could support it without issue. But changing this to even v4 would require some additional testing and it's not abundantly clear there's a huge population in the intersection of v4 + graceful disconnect.
It's also something we can change if we get pushback. A very reasonable compromise is to roll out the initial version with explicit support for v5 only and expand that to v4 if users register complaints.
I'm stealing most of this from some observations made by @SiyaoIsHiding in direct conversation recently. I believe she's going to be posting a follow-up to this effect in the not-too-distant future... I'm just noting it here since she asked me to take a look at this code. :)
--
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
Re: [PR] CASSANDRA-21191: [CEP-59] Implementation of In-Band Connection Draining (Graceful Disconnect) [cassandra]
Posted by "SiyaoIsHiding (via GitHub)" <gi...@apache.org>.
SiyaoIsHiding commented on PR #4953:
URL: https://github.com/apache/cassandra/pull/4953#issuecomment-5501543249
Please refer to https://github.com/SiyaoIsHiding/cassandra/blob/feature/cep-59/metrics/src/java/org/apache/cassandra/transport/Server.java
On how to implement the draining logic in a much simpler way, without the life cycle class.
--
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
Re: [PR] CASSANDRA-21191: [CEP-59] Implementation of In-Band Connection Draining (Graceful Disconnect) [cassandra]
Posted by "AshenScribe (via GitHub)" <gi...@apache.org>.
AshenScribe commented on PR #4953:
URL: https://github.com/apache/cassandra/pull/4953#issuecomment-6186864168
marked the comments as resolved (also did it)
--
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
Re: [PR] cep 59 [cassandra]
Posted by "SiyaoIsHiding (via GitHub)" <gi...@apache.org>.
SiyaoIsHiding commented on code in PR #4953:
URL: https://github.com/apache/cassandra/pull/4953#discussion_r3780377736
##########
src/java/org/apache/cassandra/metrics/ClientMetrics.java:
##########
@@ -71,6 +71,10 @@ public final class ClientMetrics
@VisibleForTesting
Gauge<Integer> connectedNativeClients;
+ public AtomicInteger connectionsDraining;
Review Comment:
We need tests for metrics too
##########
src/java/org/apache/cassandra/transport/SimpleClient.java:
##########
@@ -238,8 +240,9 @@ public SimpleClient connect(boolean useCompression, boolean throwOnOverload) thr
options.put(StartupMessage.COMPRESSION, "LZ4");
connection.setCompressor(Compressor.LZ4Compressor.instance);
}
+ if (version.isGreaterOrEqualTo(ProtocolVersion.V5))
+ options.put(StartupMessage.GRACEFUL_DISCONNECT, "GRACEFUL_DISCONNECT");
Review Comment:
graceful disconnect is opted in by REGISTER, not in startup message
##########
src/java/org/apache/cassandra/service/StorageService.java:
##########
@@ -3865,9 +3898,112 @@ public String getDrainProgress()
/**
* Shuts node off to writes, empties memtables and the commit log.
*/
- public synchronized void drain() throws IOException, InterruptedException, ExecutionException
+ public void drain() throws IOException, InterruptedException, ExecutionException, TimeoutException
+ {
+ if (!drainLock.tryLock())
+ {
+ throw new IllegalStateException("Drain already in progress");
+ }
+ try
+ {
+ CountDownLatch drainComplete = new CountDownLatch(1);
+ AtomicReference<Exception> failure = new AtomicReference<>();
+ gracefulDisconnect(() -> {
+ try
+ {
+ drain(false);
+ }
+ catch (IOException | InterruptedException | ExecutionException e)
+ {
+ failure.set(e);
+ }
+ finally
+ {
+ drainComplete.countDown();
+ }
+ });
+ if (!drainComplete.await(DatabaseDescriptor.getGracefulDisconnectGracePeriod(), MILLISECONDS))
+ throw new TimeoutException("Timed out waiting for drain to complete after graceful disconnect");
Review Comment:
Why does `drain` throws a TimeoutException when graceful period passes? it should at most log a warning and proceed IMO
##########
src/java/org/apache/cassandra/transport/InitialConnectionHandler.java:
##########
@@ -90,6 +91,7 @@ protected void decode(ChannelHandlerContext ctx, ByteBuf buffer, List<Object> li
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())));
Review Comment:
+1
--
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
Re: [PR] CASSANDRA-21191: [CEP-59] Implementation of In-Band Connection Draining (Graceful Disconnect) [cassandra]
Posted by "SiyaoIsHiding (via GitHub)" <gi...@apache.org>.
SiyaoIsHiding commented on PR #4953:
URL: https://github.com/apache/cassandra/pull/4953#issuecomment-5336967574
@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.
--
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
Re: [PR] CASSANDRA-21191: [CEP-59] Implementation of In-Band Connection Draining (Graceful Disconnect) [cassandra]
Posted by "SiyaoIsHiding (via GitHub)" <gi...@apache.org>.
SiyaoIsHiding commented on PR #4953:
URL: https://github.com/apache/cassandra/pull/4953#issuecomment-5415610888
Both your goal and Shanzita's goal is to eliminate errors on the driver's side, cuz that's this CEP's goal. So, you should check client side logs too cuz even if the server log shows what you expected, are you 100% sure the client side errors are gone? If you see the client side error persisting, are you 100% sure it's not the server side that's not behaving as you expected?
Re: multi-node, when you check the client side logs of your testing scenarios you will see multi-node clusters are easiest to check whether the client side errors are gone, cuz it's the difference of an application exiting or not. It's ultimately your choice tho.
--
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
Re: [PR] CASSANDRA-21191: [CEP-59] Implementation of In-Band Connection Draining (Graceful Disconnect) [cassandra]
Posted by "AshenScribe (via GitHub)" <gi...@apache.org>.
AshenScribe commented on code in PR #4953:
URL: https://github.com/apache/cassandra/pull/4953#discussion_r3922063889
##########
src/java/org/apache/cassandra/config/Config.java:
##########
@@ -143,6 +143,10 @@ public static Set<String> splitCommaDelimited(String src)
/** Triggers automatic allocation of tokens if set, based on the provided replica count for a datacenter */
public Integer allocate_tokens_for_local_replication_factor = null;
+ public boolean graceful_disconnect_enabled = false;
+
+ public volatile DurationSpec.LongMillisecondsBound graceful_disconnect_grace_period = new DurationSpec.LongMillisecondsBound(5000);
Review Comment:
Isn't this the default configuration itself?
--
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
Re: [PR] CASSANDRA-21191: [CEP-59] Implementation of In-Band Connection Draining (Graceful Disconnect) [cassandra]
Posted by "AshenScribe (via GitHub)" <gi...@apache.org>.
AshenScribe commented on code in PR #4953:
URL: https://github.com/apache/cassandra/pull/4953#discussion_r4341393104
##########
doc/modules/cassandra/pages/managing/operating/graceful_disconnect.adoc:
##########
@@ -0,0 +1,91 @@
+= Graceful Disconnect — In-Band Connection Draining for Cassandra Node Shutdown
Review Comment:
you say "manual is user facing"
I am saying manual is not need for such a popular feature. features like compaction which are very unique/customized to cassandra like [compression](https://github.com/apache/cassandra/blob/trunk/doc/modules/cassandra/pages/managing/operating/compression.adoc).
Graceful disconnect is rather straight forward, and (as per me) doesn't deserve its own manual.
--
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
Re: [PR] CASSANDRA-21191: [CEP-59] Implementation of In-Band Connection Draining (Graceful Disconnect) [cassandra]
Posted by "SiyaoIsHiding (via GitHub)" <gi...@apache.org>.
SiyaoIsHiding commented on PR #4953:
URL: https://github.com/apache/cassandra/pull/4953#issuecomment-5544708701
In `Dispatcher.java`
```java
public <P> void dispatch(Channel channel, Message.Request request, FlushItemConverter<P> forFlusher, P param, Overload backpressure)
{
if (!request.connection().getTracker().isRunning())
{
// We can not respond with a custom, transport, or server exceptions since, given current implementation of clients,
// they will defunct the connection. Without a protocol version bump that introduces an "I am going away message",
// we have to stick to an existing error code.
Message.Response response = ErrorMessage.fromTransportException(new OverloadedException("Server is shutting down"));
```
In current implementation, even with graceful enabled, the server can return `OverloadedException` because `isRunning()` will be false during the grace period.
Should change it so it processes requests normally instead of `OverloadedException` during grace period.
--
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
Re: [PR] CASSANDRA-21191: [CEP-59] Implementation of In-Band Connection Draining (Graceful Disconnect) [cassandra]
Posted by "Shanzita (via GitHub)" <gi...@apache.org>.
Shanzita commented on code in PR #4953:
URL: https://github.com/apache/cassandra/pull/4953#discussion_r3815728439
##########
src/java/org/apache/cassandra/service/StorageService.java:
##########
@@ -40,13 +40,17 @@
import java.util.UUID;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.CopyOnWriteArrayList;
+import org.apache.cassandra.utils.concurrent.CountDownLatch
Review Comment:
I think you missed a semicolon at the end of this import.
--
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
Re: [PR] CASSANDRA-21191: [CEP-59] Implementation of In-Band Connection Draining (Graceful Disconnect) [cassandra]
Posted by "SiyaoIsHiding (via GitHub)" <gi...@apache.org>.
SiyaoIsHiding commented on PR #4953:
URL: https://github.com/apache/cassandra/pull/4953#issuecomment-6162764117
It's been 3 weeks since I reviewed last time. Any update?
--
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
Re: [PR] CASSANDRA-21191: [CEP-59] Implementation of In-Band Connection Draining (Graceful Disconnect) [cassandra]
Posted by "SiyaoIsHiding (via GitHub)" <gi...@apache.org>.
SiyaoIsHiding commented on PR #4953:
URL: https://github.com/apache/cassandra/pull/4953#issuecomment-6185640689
You planning on addressing the other comments I made from the last review?
--
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
Re: [PR] CASSANDRA-21191: [CEP-59] Implementation of In-Band Connection Draining (Graceful Disconnect) [cassandra]
Posted by "SiyaoIsHiding (via GitHub)" <gi...@apache.org>.
SiyaoIsHiding commented on code in PR #4953:
URL: https://github.com/apache/cassandra/pull/4953#discussion_r4341958107
##########
doc/modules/cassandra/pages/managing/operating/graceful_disconnect.adoc:
##########
@@ -0,0 +1,91 @@
+= Graceful Disconnect — In-Band Connection Draining for Cassandra Node Shutdown
Review Comment:
Anyway, this isn't the most important thing in this PR right now. If you still don't want to add a page to the manual, please go ahead with the rest of the development, especially the `gracefulDisconnect()` & `OverloadedException` problem. We can get back to this conversation when other committers review.
--
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
Re: [PR] CASSANDRA-21191: [CEP-59] Implementation of In-Band Connection Draining (Graceful Disconnect) [cassandra]
Posted by "SiyaoIsHiding (via GitHub)" <gi...@apache.org>.
SiyaoIsHiding commented on PR #4953:
URL: https://github.com/apache/cassandra/pull/4953#issuecomment-5337657182
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.
--
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
Re: [PR] CASSANDRA-21191: [CEP-59] Implementation of In-Band Connection Draining (Graceful Disconnect) [cassandra]
Posted by "SiyaoIsHiding (via GitHub)" <gi...@apache.org>.
SiyaoIsHiding commented on code in PR #4953:
URL: https://github.com/apache/cassandra/pull/4953#discussion_r3856791124
##########
src/java/org/apache/cassandra/service/NativeTransportService.java:
##########
@@ -163,6 +164,11 @@ Server getServer()
return server;
}
+ public ChannelGroup getChannelsSubscribedToGracefulDisconnect()
Review Comment:
This is only called in tests but not in source so should be removed
--
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
Re: [PR] CASSANDRA-21191: [CEP-59] Implementation of In-Band Connection Draining (Graceful Disconnect) [cassandra]
Posted by "gandherva (via GitHub)" <gi...@apache.org>.
gandherva commented on code in PR #4953:
URL: https://github.com/apache/cassandra/pull/4953#discussion_r4118035620
##########
src/java/org/apache/cassandra/service/NativeTransportService.java:
##########
@@ -163,6 +163,12 @@ Server getServer()
return server;
}
+ @VisibleForTesting
Review Comment:
com.google.common.annotations.VisibleForTesting based on my checks has no effect on access at runtime and is only for documentation. (it also gives IDE/Static analysis something to flag production code calling a test only method).
based on a search it seems around 667 times of our 1265 times in Cassandra Repo it has been used on public.
<img width="737" height="192" alt="Image" src="https://github.com/user-attachments/assets/50c2fa02-e1d7-4238-a5ef-2c6588879cc5" />
--
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
Re: [PR] CASSANDRA-21191: [CEP-59] Implementation of In-Band Connection Draining (Graceful Disconnect) [cassandra]
Posted by "SiyaoIsHiding (via GitHub)" <gi...@apache.org>.
SiyaoIsHiding commented on code in PR #4953:
URL: https://github.com/apache/cassandra/pull/4953#discussion_r4286191987
##########
doc/native_protocol_v5.spec:
##########
@@ -1534,4 +1551,4 @@ Table of Contents
* Added [int] flags field in PREPARE message (Section 4.1.5).
* Removed NO_COMPACT startup option (Section 4.1.1.)
* Introduces outer framing format wrapping the "frames" of v4 and earlier, which are
- now referred to as "envelopes" (Sections 2.1, 2.2 and 2.3)
+ now referred to as "envelopes" (Sections 2.1, 2.2 and 2.3)
Review Comment:
remove this change
##########
doc/modules/cassandra/pages/managing/operating/graceful_disconnect.adoc:
##########
@@ -0,0 +1,91 @@
+= Graceful Disconnect — In-Band Connection Draining for Cassandra Node Shutdown
+
+== Vocabulary
+
+in-band:: same connection
+In-flight requests:: requests that have been sent but not yet completed
+
+== Introduction
+
+When a Cassandra node has to be taken offline client drivers have no reliable, in-band signal that the node is going away.
+
+* Drivers keep sending requests to a shutting-down node until they hit a socket close or timeout.
+* In-flight requests are abandoned, producing `ReadTimeoutException` errors on the client side.
Review Comment:
Not `ReadTimeoutException`, that's an exception thrown by the server. Driver usually throws `DriverTimeoutException` or a socket timeout exception, or an OverloadedException. It depends. "Client visible timeout" should be correct
##########
doc/modules/cassandra/pages/managing/operating/graceful_disconnect.adoc:
##########
@@ -0,0 +1,91 @@
+= Graceful Disconnect — In-Band Connection Draining for Cassandra Node Shutdown
+
+== Vocabulary
+
+in-band:: same connection
+In-flight requests:: requests that have been sent but not yet completed
Review Comment:
```suggestion
In-flight requests:: requests that a client has sent but has not received the response
```
##########
doc/modules/cassandra/pages/managing/operating/graceful_disconnect.adoc:
##########
@@ -0,0 +1,91 @@
+= Graceful Disconnect — In-Band Connection Draining for Cassandra Node Shutdown
+
+== Vocabulary
+
+in-band:: same connection
+In-flight requests:: requests that have been sent but not yet completed
+
+== Introduction
+
+When a Cassandra node has to be taken offline client drivers have no reliable, in-band signal that the node is going away.
+
+* Drivers keep sending requests to a shutting-down node until they hit a socket close or timeout.
+* In-flight requests are abandoned, producing `ReadTimeoutException` errors on the client side.
+* Retry storms emerge as clients simultaneously rediscover the topology.
+
+== 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:
Review Comment:
```suggestion
Introduce an in-band EVENT — `GRACEFUL_DISCONNECT` — so that the server can notify clients (that have subscribed) connection before closing it. This gives drivers time to:
```
##########
doc/modules/cassandra/pages/managing/operating/graceful_disconnect.adoc:
##########
@@ -0,0 +1,91 @@
+= Graceful Disconnect — In-Band Connection Draining for Cassandra Node Shutdown
+
+== Vocabulary
+
+in-band:: same connection
+In-flight requests:: requests that have been sent but not yet completed
+
+== Introduction
+
+When a Cassandra node has to be taken offline client drivers have no reliable, in-band signal that the node is going away.
+
+* Drivers keep sending requests to a shutting-down node until they hit a socket close or timeout.
+* In-flight requests are abandoned, producing `ReadTimeoutException` errors on the client side.
+* Retry storms emerge as clients simultaneously rediscover the topology.
Review Comment:
drivers will still retry the same way
##########
doc/modules/cassandra/pages/managing/operating/graceful_disconnect.adoc:
##########
@@ -0,0 +1,91 @@
+= Graceful Disconnect — In-Band Connection Draining for Cassandra Node Shutdown
+
+== Vocabulary
+
+in-band:: same connection
+In-flight requests:: requests that have been sent but not yet completed
+
+== Introduction
+
+When a Cassandra node has to be taken offline client drivers have no reliable, in-band signal that the node is going away.
+
+* Drivers keep sending requests to a shutting-down node until they hit a socket close or timeout.
+* In-flight requests are abandoned, producing `ReadTimeoutException` errors on the client side.
+* Retry storms emerge as clients simultaneously rediscover the topology.
+
+== 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:
+
+. Stop sending new requests on that connection/node.
+. Let all in-flight requests complete.
+. Close that socket connection (this notifies server that there are no pending queries on client side).
+. Try reconnecting with exponential backoff.
Review Comment:
No different behavior in reconnecting with exponential backoff
##########
doc/modules/cassandra/pages/managing/operating/graceful_disconnect.adoc:
##########
@@ -0,0 +1,91 @@
+= Graceful Disconnect — In-Band Connection Draining for Cassandra Node Shutdown
+
+== Vocabulary
+
+in-band:: same connection
+In-flight requests:: requests that have been sent but not yet completed
+
+== Introduction
+
+When a Cassandra node has to be taken offline client drivers have no reliable, in-band signal that the node is going away.
+
+* Drivers keep sending requests to a shutting-down node until they hit a socket close or timeout.
+* In-flight requests are abandoned, producing `ReadTimeoutException` errors on the client side.
+* Retry storms emerge as clients simultaneously rediscover the topology.
+
+== 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:
+
+. Stop sending new requests on that connection/node.
+. Let all in-flight requests complete.
+. Close that socket connection (this notifies server that there are no pending queries on client side).
+. Try reconnecting with exponential backoff.
+
+== New Configurations Introduced
+
+`graceful_disconnect_enabled`:: A configuration that enables the server to perform graceful disconnect.
+`graceful_disconnect_grace_period`:: A configuration that forces shutdown of any active driver connection to the node after the grace period expires.
+
+[cols="1,1,3,1", options="header"]
+|===
+| Parameter | Type | Description | Default
+| `graceful_disconnect_enabled` | Boolean | A configuration that enables server to perform graceful disconnect. | false
+| `graceful_disconnect_grace_period` | Duration (ms) | A configuration that determines after how much time to force close a socket connection, if there is any pending connection from driver side. | 5s
+|===
+
+== Client Compatibility & Upgrade Strategy
+
+=== Legacy Drivers — No Change Required
+
+Drivers that do not support graceful disconnect are not affected by any value of `graceful_disconnect_enabled` or `graceful_disconnect_grace_period`.
+
+=== Required Driver Versions
+
+To benefit from graceful draining, a compatible driver must be used.
+
+[cols="1,1", options="header"]
+|===
+| Driver | Status
+| Java | https://issues.apache.org/jira/browse/CASSJAVA-124[In progress]
+| Python | https://issues.apache.org/jira/browse/CASSPYTHON-16[In progress]
+| Node.js | https://issues.apache.org/jira/browse/CASSNODEJS-5[In progress]
+| Go | https://issues.apache.org/jira/browse/CASSGO-117[In progress]
+| C++ | https://issues.apache.org/jira/browse/CASSCPP-7[In progress]
+|===
+
+=== Mixed-Fleet Rollout
+
+During a rolling upgrade where some nodes support the feature and others do not:
+
+* Nodes *with* `graceful_disconnect_enabled: true` advertise the capability in `SUPPORTED` and drivers subscribe.
+* Nodes *without* the feature omit the key from `SUPPORTED`. Compatible drivers silently fall back to the legacy TCP teardown path for those nodes.
+
+No special coordination is needed. Drivers handle both peers transparently within the same session.
+
+== Operational Visibility
+
+=== Metrics
+
+[cols="1,3", options="header"]
+|===
+| Metric | Description
+| `ConnectionsDraining` | Current count of connections in the Draining state (subscribed and awaiting in-flight completion). Should rise at drain start and fall to zero before shutdown completes.
+| `ForcedDisconnects` | Cumulative count of connections force-closed after `graceful_disconnect_grace_period` expired without the driver closing cleanly. Persistent non-zero values here indicate driver-side issues or an undersized grace period.
+|===
+
+== Failure Modes & Edge Cases
+
+=== What if the driver does not support `GRACEFUL_DISCONNECT`?
+
+If the driver never sent `REGISTER` for this event type (legacy driver, or a compatible driver connected to a legacy node), the server applies *standard TCP teardown* — the same behavior as before this feature existed. There is no error, no retry storm specific to this feature, and no action required from operators.
Review Comment:
"no retry storm specific to this feature," what does this mean
##########
doc/modules/cassandra/pages/managing/operating/graceful_disconnect.adoc:
##########
@@ -0,0 +1,91 @@
+= Graceful Disconnect — In-Band Connection Draining for Cassandra Node Shutdown
+
+== Vocabulary
+
+in-band:: same connection
+In-flight requests:: requests that have been sent but not yet completed
+
+== Introduction
+
+When a Cassandra node has to be taken offline client drivers have no reliable, in-band signal that the node is going away.
+
+* Drivers keep sending requests to a shutting-down node until they hit a socket close or timeout.
+* In-flight requests are abandoned, producing `ReadTimeoutException` errors on the client side.
+* Retry storms emerge as clients simultaneously rediscover the topology.
+
+== 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:
+
+. Stop sending new requests on that connection/node.
+. Let all in-flight requests complete.
+. Close that socket connection (this notifies server that there are no pending queries on client side).
+. Try reconnecting with exponential backoff.
+
+== New Configurations Introduced
+
+`graceful_disconnect_enabled`:: A configuration that enables the server to perform graceful disconnect.
+`graceful_disconnect_grace_period`:: A configuration that forces shutdown of any active driver connection to the node after the grace period expires.
+
+[cols="1,1,3,1", options="header"]
+|===
+| Parameter | Type | Description | Default
+| `graceful_disconnect_enabled` | Boolean | A configuration that enables server to perform graceful disconnect. | false
+| `graceful_disconnect_grace_period` | Duration (ms) | A configuration that determines after how much time to force close a socket connection, if there is any pending connection from driver side. | 5s
+|===
+
+== Client Compatibility & Upgrade Strategy
+
+=== Legacy Drivers — No Change Required
+
+Drivers that do not support graceful disconnect are not affected by any value of `graceful_disconnect_enabled` or `graceful_disconnect_grace_period`.
+
+=== Required Driver Versions
+
+To benefit from graceful draining, a compatible driver must be used.
+
+[cols="1,1", options="header"]
+|===
+| Driver | Status
+| Java | https://issues.apache.org/jira/browse/CASSJAVA-124[In progress]
+| Python | https://issues.apache.org/jira/browse/CASSPYTHON-16[In progress]
Review Comment:
Python, node.js, and Go have not started
##########
doc/modules/cassandra/pages/managing/operating/graceful_disconnect.adoc:
##########
@@ -0,0 +1,91 @@
+= Graceful Disconnect — In-Band Connection Draining for Cassandra Node Shutdown
+
+== Vocabulary
+
+in-band:: same connection
+In-flight requests:: requests that have been sent but not yet completed
+
+== Introduction
+
+When a Cassandra node has to be taken offline client drivers have no reliable, in-band signal that the node is going away.
+
+* Drivers keep sending requests to a shutting-down node until they hit a socket close or timeout.
+* In-flight requests are abandoned, producing `ReadTimeoutException` errors on the client side.
+* Retry storms emerge as clients simultaneously rediscover the topology.
+
+== 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:
+
+. Stop sending new requests on that connection/node.
+. Let all in-flight requests complete.
+. Close that socket connection (this notifies server that there are no pending queries on client side).
+. Try reconnecting with exponential backoff.
+
+== New Configurations Introduced
+
+`graceful_disconnect_enabled`:: A configuration that enables the server to perform graceful disconnect.
+`graceful_disconnect_grace_period`:: A configuration that forces shutdown of any active driver connection to the node after the grace period expires.
+
+[cols="1,1,3,1", options="header"]
+|===
+| Parameter | Type | Description | Default
+| `graceful_disconnect_enabled` | Boolean | A configuration that enables server to perform graceful disconnect. | false
+| `graceful_disconnect_grace_period` | Duration (ms) | A configuration that determines after how much time to force close a socket connection, if there is any pending connection from driver side. | 5s
+|===
+
+== Client Compatibility & Upgrade Strategy
+
+=== Legacy Drivers — No Change Required
+
+Drivers that do not support graceful disconnect are not affected by any value of `graceful_disconnect_enabled` or `graceful_disconnect_grace_period`.
+
+=== Required Driver Versions
+
+To benefit from graceful draining, a compatible driver must be used.
+
+[cols="1,1", options="header"]
+|===
+| Driver | Status
+| Java | https://issues.apache.org/jira/browse/CASSJAVA-124[In progress]
+| Python | https://issues.apache.org/jira/browse/CASSPYTHON-16[In progress]
+| Node.js | https://issues.apache.org/jira/browse/CASSNODEJS-5[In progress]
+| Go | https://issues.apache.org/jira/browse/CASSGO-117[In progress]
+| C++ | https://issues.apache.org/jira/browse/CASSCPP-7[In progress]
+|===
+
+=== Mixed-Fleet Rollout
+
+During a rolling upgrade where some nodes support the feature and others do not:
+
+* Nodes *with* `graceful_disconnect_enabled: true` advertise the capability in `SUPPORTED` and drivers subscribe.
+* Nodes *without* the feature omit the key from `SUPPORTED`. Compatible drivers silently fall back to the legacy TCP teardown path for those nodes.
+
+No special coordination is needed. Drivers handle both peers transparently within the same session.
+
+== Operational Visibility
+
+=== Metrics
+
+[cols="1,3", options="header"]
+|===
+| Metric | Description
+| `ConnectionsDraining` | Current count of connections in the Draining state (subscribed and awaiting in-flight completion). Should rise at drain start and fall to zero before shutdown completes.
+| `ForcedDisconnects` | Cumulative count of connections force-closed after `graceful_disconnect_grace_period` expired without the driver closing cleanly. Persistent non-zero values here indicate driver-side issues or an undersized grace period.
+|===
+
+== Failure Modes & Edge Cases
+
+=== What if the driver does not support `GRACEFUL_DISCONNECT`?
+
+If the driver never sent `REGISTER` for this event type (legacy driver, or a compatible driver connected to a legacy node), the server applies *standard TCP teardown* — the same behavior as before this feature existed. There is no error, no retry storm specific to this feature, and no action required from operators.
+
+This is the designed fallback and not an error condition.
+
+=== What will happen if two different driver, one supporting graceful disconnect while other not, are talking to a node supporting graceful disconnect?
+
+Driver supporting graceful disconnect will disconnect gracefully, while the one not supporting graceful disconnect will behave as a legacy driver.
+
+=== What if the server crashes instead of draining cleanly?
+
+`GRACEFUL_DISCONNECT` is *not* a crash-safety mechanism. If the JVM is killed with `SIGKILL`, the node loses power, or an OOM kill occurs, no `GRACEFUL_DISCONNECT` event is emitted. Drivers fall back to socket-close detection and gossip `DOWN` events — the current behavior.
Review Comment:
As a standalone section, not just as edge cases
##########
conf/cassandra.yaml:
##########
@@ -1134,6 +1104,19 @@ native_transport_allow_older_protocols: true
# native_transport_rate_limiting_enabled: false
# native_transport_max_requests_per_second: 1000000
+# When enabled, nodes will signal connected clients before shutting down,
+# allowing in-flight requests to complete without client-visible timeouts.
+# This applies to intentional shutdowns (nodetool drain, rolling restarts,
+# controlled JVM shutdown). Clients must subscribe to the GRACEFUL_DISCONNECT
+# event via REGISTER to benefit from this behavior.
+# Requires driver support for the GRACEFUL_DISCONNECT event type.
+# See: doc/modules/cassandra/pages/managing/operating/graceful_disconnect.adoc
+# Defaults to false.
+# graceful_disconnect_enabled: false
+
+# Time given to clients to stop sending new requests after the GRACEFUL_DISCONNECT event is emitted.
+# graceful_disconnect_grace_period: 5s
Review Comment:
See my previous review on graceful disconnect grace period longer than default request timeout.
##########
doc/modules/cassandra/pages/managing/operating/graceful_disconnect.adoc:
##########
@@ -0,0 +1,91 @@
+= Graceful Disconnect — In-Band Connection Draining for Cassandra Node Shutdown
Review Comment:
There were discussions in the mailing list to completely ban AI-generated doc or comments cuz a lot of committers find it low quality. Though that didn't become a formal policy last time I checked, we should scrutinize your doc and comments in this PR.
On the good side, you can largely copy from the [CEP](https://cwiki.apache.org/confluence/spaces/CASSANDRA/pages/406619103/CEP-59+Graceful+Disconnect+%E2%80%93+In-Band+Connection+Draining+for+Node+Shutdown).
##########
src/java/org/apache/cassandra/transport/Server.java:
##########
@@ -202,6 +269,11 @@ private void close(boolean force)
{
if (!force)
{
+ if (DatabaseDescriptor.getGracefulDisconnectEnabled())
+ gracefulDisconnect();
+ else
+ stopAcceptingNewConnections();
Review Comment:
I believe you made a mistake when merging my commit.
Our goal:
1. `gracefulDisconnect` should only be called once, either from `stop()` or `close()`, `close()` is only called from `stop()` anyway.
2. We need to make sure only one gracefulDisconnect is happening per Server, and at most once, not back to back like this.
3. Stop throwing OverloadedException as long as graceful disconnect is on.
##########
doc/modules/cassandra/pages/managing/operating/graceful_disconnect.adoc:
##########
@@ -0,0 +1,91 @@
+= Graceful Disconnect — In-Band Connection Draining for Cassandra Node Shutdown
+
+== Vocabulary
+
+in-band:: same connection
+In-flight requests:: requests that have been sent but not yet completed
+
+== Introduction
+
+When a Cassandra node has to be taken offline client drivers have no reliable, in-band signal that the node is going away.
+
+* Drivers keep sending requests to a shutting-down node until they hit a socket close or timeout.
+* In-flight requests are abandoned, producing `ReadTimeoutException` errors on the client side.
+* Retry storms emerge as clients simultaneously rediscover the topology.
+
+== 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:
+
+. Stop sending new requests on that connection/node.
+. Let all in-flight requests complete.
+. Close that socket connection (this notifies server that there are no pending queries on client side).
+. Try reconnecting with exponential backoff.
+
+== New Configurations Introduced
+
+`graceful_disconnect_enabled`:: A configuration that enables the server to perform graceful disconnect.
+`graceful_disconnect_grace_period`:: A configuration that forces shutdown of any active driver connection to the node after the grace period expires.
+
+[cols="1,1,3,1", options="header"]
+|===
+| Parameter | Type | Description | Default
+| `graceful_disconnect_enabled` | Boolean | A configuration that enables server to perform graceful disconnect. | false
+| `graceful_disconnect_grace_period` | Duration (ms) | A configuration that determines after how much time to force close a socket connection, if there is any pending connection from driver side. | 5s
+|===
+
+== Client Compatibility & Upgrade Strategy
+
+=== Legacy Drivers — No Change Required
+
+Drivers that do not support graceful disconnect are not affected by any value of `graceful_disconnect_enabled` or `graceful_disconnect_grace_period`.
+
+=== Required Driver Versions
+
+To benefit from graceful draining, a compatible driver must be used.
+
+[cols="1,1", options="header"]
+|===
+| Driver | Status
+| Java | https://issues.apache.org/jira/browse/CASSJAVA-124[In progress]
+| Python | https://issues.apache.org/jira/browse/CASSPYTHON-16[In progress]
+| Node.js | https://issues.apache.org/jira/browse/CASSNODEJS-5[In progress]
+| Go | https://issues.apache.org/jira/browse/CASSGO-117[In progress]
+| C++ | https://issues.apache.org/jira/browse/CASSCPP-7[In progress]
+|===
+
+=== Mixed-Fleet Rollout
+
+During a rolling upgrade where some nodes support the feature and others do not:
+
+* Nodes *with* `graceful_disconnect_enabled: true` advertise the capability in `SUPPORTED` and drivers subscribe.
+* Nodes *without* the feature omit the key from `SUPPORTED`. Compatible drivers silently fall back to the legacy TCP teardown path for those nodes.
+
+No special coordination is needed. Drivers handle both peers transparently within the same session.
+
+== Operational Visibility
+
+=== Metrics
+
+[cols="1,3", options="header"]
+|===
+| Metric | Description
+| `ConnectionsDraining` | Current count of connections in the Draining state (subscribed and awaiting in-flight completion). Should rise at drain start and fall to zero before shutdown completes.
+| `ForcedDisconnects` | Cumulative count of connections force-closed after `graceful_disconnect_grace_period` expired without the driver closing cleanly. Persistent non-zero values here indicate driver-side issues or an undersized grace period.
+|===
+
+== Failure Modes & Edge Cases
+
+=== What if the driver does not support `GRACEFUL_DISCONNECT`?
+
+If the driver never sent `REGISTER` for this event type (legacy driver, or a compatible driver connected to a legacy node), the server applies *standard TCP teardown* — the same behavior as before this feature existed. There is no error, no retry storm specific to this feature, and no action required from operators.
+
+This is the designed fallback and not an error condition.
+
+=== What will happen if two different driver, one supporting graceful disconnect while other not, are talking to a node supporting graceful disconnect?
+
+Driver supporting graceful disconnect will disconnect gracefully, while the one not supporting graceful disconnect will behave as a legacy driver.
+
+=== What if the server crashes instead of draining cleanly?
+
+`GRACEFUL_DISCONNECT` is *not* a crash-safety mechanism. If the JVM is killed with `SIGKILL`, the node loses power, or an OOM kill occurs, no `GRACEFUL_DISCONNECT` event is emitted. Drivers fall back to socket-close detection and gossip `DOWN` events — the current behavior.
Review Comment:
```
Such server shutdown behavior applies as follows.
nodetool disablebinary yes
nodetool disablebinary -f ("Use -f to interrupt client requests that have already started") no
nodetool drain yes
nodetool stopdaemon yes
nodetool decommission yes
nodetool decommission -f yes
JVM shutdown hook/ SIGTERM/ systemctl stop yes
ccm node1 stop (SIGTERM) yes
SIGKILL no
```
This part from the CEP should be pasted here.
--
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
Re: [PR] CASSANDRA-21191: [CEP-59] Implementation of In-Band Connection Draining (Graceful Disconnect) [cassandra]
Posted by "AshenScribe (via GitHub)" <gi...@apache.org>.
AshenScribe commented on code in PR #4953:
URL: https://github.com/apache/cassandra/pull/4953#discussion_r4341259705
##########
doc/modules/cassandra/pages/managing/operating/graceful_disconnect.adoc:
##########
@@ -0,0 +1,91 @@
+= Graceful Disconnect — In-Band Connection Draining for Cassandra Node Shutdown
Review Comment:
```
# Enables graceful disconnection of clients during node shutdown.
# When enabled, node stops accepting new connections, notifies compatible drivers
# to stop sending new requests to the node, allowing in-flight requests to complete before
# Connections that fail to drain within graceful_disconnect_grace_period are forcibly closed
# so shutdown can proceed. Connections that fail to drain within the configured
# grace period are forcibly closed. Requires driver support for this feature
# unsupported drivers will behave as if the flag is false.
# triggered on nodetool disablebinary/drain/stopdaemon/decommission(+ -f flag)
# for compatible drivers, see https://issues.apache.org/jira/browse/CASSANDRA-21191
# graceful_disconnect_enabled: false
# graceful_disconnect_grace_period: 15000ms
```
Graceful disconnect is not a new concept in software engineering, its used enough in http2, so operators will know what this means.
this has been stripped of all the implementation details (the driver handshake work and event emission), is this not sufficient for cassandra users/operators?
This will potentially be a better choice as the doc file current patch has, more or less aims to explain the CEP rather than what just this flag does. (I know I wrote it back then, but I was much naive when I wrote it).
--
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
Re: [PR] CASSANDRA-21191: [CEP-59] Implementation of In-Band Connection Draining (Graceful Disconnect) [cassandra]
Posted by "AshenScribe (via GitHub)" <gi...@apache.org>.
AshenScribe commented on code in PR #4953:
URL: https://github.com/apache/cassandra/pull/4953#discussion_r3922116272
##########
src/java/org/apache/cassandra/transport/Server.java:
##########
@@ -330,6 +395,8 @@ public boolean isRunning()
public void register(Event.Type type, Channel ch)
{
+ if (type == Event.Type.GRACEFUL_DISCONNECT && !DatabaseDescriptor.getGracefulDisconnectEnabled())
+ return;
Review Comment:
Should we or should we not consider case of a buggy client sending GRACEFUL_DISCONNECT in register regardless of false value of that flag in supported options.
--
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
Re: [PR] CASSANDRA-21191: [CEP-59] Implementation of In-Band Connection Draining (Graceful Disconnect) [cassandra]
Posted by "AshenScribe (via GitHub)" <gi...@apache.org>.
AshenScribe commented on PR #4953:
URL: https://github.com/apache/cassandra/pull/4953#issuecomment-5300754396
@SiyaoIsHiding I need a java driver supporting cep 59. I am unable to get one, so further testing can't be done by me.
--
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
Re: [PR] CASSANDRA-21191: [CEP-59] Implementation of In-Band Connection Draining (Graceful Disconnect) [cassandra]
Posted by "SiyaoIsHiding (via GitHub)" <gi...@apache.org>.
SiyaoIsHiding commented on PR #4953:
URL: https://github.com/apache/cassandra/pull/4953#issuecomment-5366754876
<p>Here are a list of different ways for a node to disconnect and whether graceful disconnect should kick in.</p>
Way to Disconnect | Graceful disconnect?
-- | --
nodetool disablebinary | yes
nodetool disablebinary -f ("Use -f to interrupt client requests that have already started") | no
nodetool drain | yes
nodetool stopdaemon | yes
nodetool decommission | yes
nodetool decommission -f | yes
JVM shutdown hook/ SIGTERM/ systemctl stop | yes
ccm node1 stop (SIGTERM) | yes
SIGKILL | no
<!-- obsidian --><p>If everyone agree with the above behavior, this table should go into the CEP and the operating manual.</p>
<p>Assuming we want the above behavior, I'd suggest we add graceful disconnect to <code>NativeTransportService.stop(bool force)</code> when <code>force == false</code>, because all ways above that should gracefully disconnect calls <code>NativeTransportService.stop(false)</code>.</p>
Way to Disconnect | Callstack to graceful disconnect
-- | --
nodetool disablebinary | StorageService.stopNativeTransport(bool force) -> CassandraDaemon.stopNativeTransport(bool force) -> NativeTransportService.stop(bool force)
nodetool disablebinary -f | Same as above, and it calls NativeTransportService.stop(true), so it should skip the graceful disconnect
nodetool drain | StorageService.drain(bool) -> StorageService.shutdownClientServers() -> StorageService.stopNativeTransport(false)
nodetool stopdaemon | CassandraDaemon.stop() -> CassandraDaemon.destroyClientTransports() -> NativeTransportService.stop()
nodetool decommission | StorageService.decommission() -> SingleNodeSequences.decommission(true, force) -> StorageService.shutdownNetowrking() -> StorageService.shutdownClientServers()
nodetool decommission -f | Exactly same above
JVM shutdown hook/ SIGTERM/ systemctl stop | Runtime.getRuntime().addShutdownHook(drainOnShutdown); -> StorageService.drain(true) -> shutdownClientServers()
SIGKILL | not handled
--
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
Re: [PR] CASSANDRA-21191: [CEP-59] Implementation of In-Band Connection Draining (Graceful Disconnect) [cassandra]
Posted by "AshenScribe (via GitHub)" <gi...@apache.org>.
AshenScribe commented on code in PR #4953:
URL: https://github.com/apache/cassandra/pull/4953#discussion_r4341023502
##########
doc/modules/cassandra/pages/managing/operating/graceful_disconnect.adoc:
##########
@@ -0,0 +1,91 @@
+= Graceful Disconnect — In-Band Connection Draining for Cassandra Node Shutdown
Review Comment:
What does the ticket lacks that a doc file is needed? The **summary** of this feature is well defined in `cassandra.yaml` and a slight more descriptive version is on ticket, if you want more details version, the original CEP text is the best source.
So copy pasting most the text doesn't makes sense to me.
--
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
Re: [PR] CASSANDRA-21191: [CEP-59] Implementation of In-Band Connection Draining (Graceful Disconnect) [cassandra]
Posted by "AshenScribe (via GitHub)" <gi...@apache.org>.
AshenScribe commented on code in PR #4953:
URL: https://github.com/apache/cassandra/pull/4953#discussion_r4341363244
##########
doc/modules/cassandra/pages/managing/operating/graceful_disconnect.adoc:
##########
@@ -0,0 +1,91 @@
+= Graceful Disconnect — In-Band Connection Draining for Cassandra Node Shutdown
Review Comment:
Yes, operators will resort to jira if they need any specific information, like which drivers support graceful disconnect. I think jira is best source for it.
As let's Go driver supports/planning to support graceful disconnect, then start a PR to change doc's WAITING to IN PROGRESS, or doc's IN PROGRESS to SUPPORTED
--
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
Re: [PR] CASSANDRA-21191: [CEP-59] Implementation of In-Band Connection Draining (Graceful Disconnect) [cassandra]
Posted by "absurdfarce (via GitHub)" <gi...@apache.org>.
absurdfarce commented on code in PR #4953:
URL: https://github.com/apache/cassandra/pull/4953#discussion_r4353184282
##########
doc/modules/cassandra/pages/managing/operating/graceful_disconnect.adoc:
##########
@@ -0,0 +1,91 @@
+= Graceful Disconnect — In-Band Connection Draining for Cassandra Node Shutdown
Review Comment:
Agreed we can wait on this until later in the development cycle but it does seem like there's some user-facing content that needs to be added to the manual here. At a bare minimum we're looking at some combination of the following:
* Which nodetool commands affect graceful disconnect (and how they affect it)
* How to monitor behaviour of graceful disconnect via metrics
I would think we'd be just fine adding a short section to, say, the ["Operating Cassandra" page](https://cassandra.apache.org/doc/latest/cassandra/managing/operating/index.html) describing how devops folk might monitor and manage graceful disconnect behaviours in their cluster.
--
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
Re: [PR] cep 59 [cassandra]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #4953:
URL: https://github.com/apache/cassandra/pull/4953#discussion_r3780055600
##########
src/java/org/apache/cassandra/transport/InitialConnectionHandler.java:
##########
@@ -90,6 +91,7 @@ protected void decode(ChannelHandlerContext ctx, ByteBuf buffer, List<Object> li
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())));
Review Comment:
The pre-negotiation OPTIONS path now includes `GRACEFUL_DISCONNECT=false` when the feature is disabled, while `OptionsMessage` omits the key and the documented compatibility contract says disabled nodes omit it. Since drivers detect support by key presence, they can attempt a V5 registration against a disabled node. Only add the capability when enabled, matching `OptionsMessage`.
##########
src/java/org/apache/cassandra/transport/SimpleClient.java:
##########
@@ -29,7 +29,7 @@
import java.util.Queue;
import java.util.concurrent.BlockingQueue;
import java.util.concurrent.ConcurrentLinkedQueue;
-import java.util.concurrent.SynchronousQueue; // checkstyle: permit this import
+import java.util.concurrent.SynchronousQueue;
Review Comment:
Removing this suppression makes `SynchronousQueue` fail the `IllegalImport` check in `.build/checkstyle.xml:104`. Restore the established import-level suppression.
This issue also appears on line 85 of the same file.
##########
src/java/org/apache/cassandra/service/StorageService.java:
##########
@@ -3859,9 +3898,112 @@ public String getDrainProgress()
/**
* Shuts node off to writes, empties memtables and the commit log.
*/
- public synchronized void drain() throws IOException, InterruptedException, ExecutionException
+ public void drain() throws IOException, InterruptedException, ExecutionException, TimeoutException
+ {
+ if (!drainLock.tryLock())
+ {
+ throw new IllegalStateException("Drain already in progress");
+ }
+ try
+ {
+ CountDownLatch drainComplete = new CountDownLatch(1);
+ AtomicReference<Exception> failure = new AtomicReference<>();
+ gracefulDisconnect(() -> {
+ try
+ {
+ drain(false);
+ }
+ catch (IOException | InterruptedException | ExecutionException e)
+ {
+ failure.set(e);
+ }
+ finally
+ {
+ drainComplete.countDown();
+ }
+ });
+ if (!drainComplete.await(DatabaseDescriptor.getGracefulDisconnectGracePeriod(), MILLISECONDS))
+ throw new TimeoutException("Timed out waiting for drain to complete after graceful disconnect");
+ Exception e = failure.get();
+ if (e instanceof IOException) throw (IOException) e;
+ if (e instanceof InterruptedException) throw (InterruptedException) e;
+ if (e instanceof ExecutionException) throw (ExecutionException) e;
+ if (e != null) throw new RuntimeException(e);
+ }
+ finally
+ {
+ drainLock.unlock();
+ }
+ }
+
+ private void gracefulDisconnect(Runnable defaultAction)
+ {
+ NativeTransportService nts = CassandraDaemon.instance.nativeTransportService();
+ if (nts == null || nts.getServer() == null)
+ {
+ defaultAction.run();
+ return;
+ }
+ nts.getServer().stopAcceptingNewConnections();
+ ChannelGroup channelGroup = nts.getServer().getChannelsSubscribedToGracefulDisconnect();
+ if (channelGroup == null || channelGroup.isEmpty())
+ {
+ defaultAction.run();
+ return;
+ }
+ gracefulDisconnect(defaultAction, channelGroup);
+ }
+
+ public void gracefulDisconnect(Runnable defaultAction, ChannelGroup channelGroup)
{
- drain(false);
+ if (channelGroup == null || channelGroup.isEmpty())
+ {
+ defaultAction.run();
+ return;
+ }
+ AtomicBoolean actionStarted = new AtomicBoolean(false);
+ AtomicInteger connectedChannels = new AtomicInteger(channelGroup.size());
+ Runnable runOnceAction = () -> {
+ if (actionStarted.compareAndSet(false, true))
+ defaultAction.run();
+ };
+ Runnable onTimeout = () -> {
+ if (actionStarted.get())
+ return;
+ int remaining = connectedChannels.get();
+ if (remaining > 0)
+ {
+ ClientMetrics.instance.markForcedDisconnect(remaining);
+ channelGroup.close();
+ }
+ else
+ {
+ runOnceAction.run();
+ }
+ };
+ ScheduledFuture<?> timeoutTask = ScheduledExecutors.nonPeriodicTasks
+ .schedule(onTimeout, DatabaseDescriptor.getGracefulDisconnectGracePeriod(), MILLISECONDS);
+ EventMessage gracefulDisconnectEventMessage = new EventMessage(new Event.GracefulDisconnect());
+ channelGroup.forEach(channel -> {
+ Consumer<EventMessage> dispatcher = channel.attr(EVENT_DISPATCHER).get();
+ if (dispatcher != null)
+ dispatcher.accept(gracefulDisconnectEventMessage);
+
+ channel.closeFuture().addListener(future -> {
+ ClientMetrics.instance.decrementConnectionsDraining();
+ if (connectedChannels.decrementAndGet() == 0)
+ {
+ timeoutTask.cancel(false);
+ runOnceAction.run();
Review Comment:
`runOnceAction` is invoked directly by this Netty close-future listener. The supplied actions include the blocking full drain and `daemon.deactivate()`; the latter ultimately calls `workerGroup.shutdownGracefully(...).awaitUninterruptibly()` on the same worker event loop, which can deadlock or raise Netty's blocking-operation error. Dispatch lifecycle work off the channel event loop (or only signal the original caller to run it).
##########
test/unit/org/apache/cassandra/service/StorageServiceTest.java:
##########
@@ -201,6 +202,32 @@ public void testColumnIndexSizeInKiB()
}
}
+ @Test
+ public void testGracefulDisconnectGracePeriod()
+ {
+ StorageService storageService = StorageService.instance;
+ long originalGracePeriod = storageService.getGracefulDisconnectGracePeriod();
+ try
+ {
+ storageService.setGracefulDisconnectGracePeriod(3000);
+ Assertions.assertThat(3000).isEqualTo(storageService.getGracefulDisconnectGracePeriod());
+
+ Assertions.assertThatThrownBy(() -> storageService.setGracefulDisconnectGracePeriod(-1)).isInstanceOf(IllegalArgumentException.class);
+
+ Assertions.assertThat(3000).isEqualTo(storageService.getGracefulDisconnectGracePeriod());
+ }
+ finally
+ {
+ storageService.setGracefulDisconnectGracePeriod(originalGracePeriod);
+ }
+ }
+
+ @Test
+ public void testGracefulDisconnectEnabled()
+ {
+ Assertions.assertThat(StorageService.instance.getGracefulDisconnectEnabled()).isFalse();
Review Comment:
This assertion depends on the loaded runtime YAML, but this PR sets `graceful_disconnect_enabled: true` in `cassandra_latest.yaml`. Consequently this test fails under Cassandra's `test-latest` target. The default-value check already belongs in `DatabaseDescriptorTest` using `new Config()`; remove this runtime-config assertion or make it honor the selected test configuration.
##########
doc/modules/cassandra/pages/managing/operating/graceful_disconnect.adoc:
##########
@@ -0,0 +1,91 @@
+= Graceful Disconnect — In-Band Connection Draining for Cassandra Node Shutdown
+
+== Vocabulary
+
+in-band:: same connection
+In-flight requests:: requests that have been sent but not yet completed
+
+== Introduction
+
+When a Cassandra node has to be taken offline client drivers have no reliable, in-band signal that the node is going away.
+
+* Drivers keep sending requests to a shutting-down node until they hit a socket close or timeout.
+* In-flight requests are abandoned, producing `ReadTimeoutException` errors on the client side.
+* Retry storms emerge as clients simultaneously rediscover the topology.
+
+== 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:
Review Comment:
This sentence is missing the preposition/object needed to describe where the notification is sent.
##########
src/java/org/apache/cassandra/transport/Server.java:
##########
@@ -157,6 +164,16 @@ public synchronized void start()
isRunning.set(true);
}
+ public void stopAcceptingNewConnections()
+ {
+ 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();
Review Comment:
Closing the bind channel removes it from `connectionTracker.allChannels`, but `ConnectionTracker.countConnectedClients()` assumes any nonempty group still contains that bind channel and returns `size - 1` (`Server.java:374-381`). Throughout the grace period this under-reports connected clients by one. Count actual `ServerConnection` channels or otherwise stop relying on the bind channel remaining in the group.
##########
src/java/org/apache/cassandra/service/StorageService.java:
##########
@@ -3859,9 +3898,112 @@ public String getDrainProgress()
/**
* Shuts node off to writes, empties memtables and the commit log.
*/
- public synchronized void drain() throws IOException, InterruptedException, ExecutionException
+ public void drain() throws IOException, InterruptedException, ExecutionException, TimeoutException
+ {
+ if (!drainLock.tryLock())
+ {
+ throw new IllegalStateException("Drain already in progress");
+ }
+ try
+ {
+ CountDownLatch drainComplete = new CountDownLatch(1);
+ AtomicReference<Exception> failure = new AtomicReference<>();
+ gracefulDisconnect(() -> {
+ try
+ {
+ drain(false);
+ }
+ catch (IOException | InterruptedException | ExecutionException e)
+ {
+ failure.set(e);
+ }
+ finally
+ {
+ drainComplete.countDown();
+ }
+ });
+ if (!drainComplete.await(DatabaseDescriptor.getGracefulDisconnectGracePeriod(), MILLISECONDS))
+ throw new TimeoutException("Timed out waiting for drain to complete after graceful disconnect");
Review Comment:
This timeout starts before the graceful phase and uses exactly the same duration as that phase. If a client waits for the full grace period, forced closure only starts at this deadline and `drain(false)` has not completed—or may not even have started—so `nodetool drain` reports a timeout while drain continues asynchronously. The grace period should bound client disconnection only; once the callback starts, preserve the synchronous drain contract by awaiting completion.
##########
src/java/org/apache/cassandra/service/StorageService.java:
##########
@@ -40,13 +40,17 @@
import java.util.UUID;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.CopyOnWriteArrayList;
+import java.util.concurrent.CountDownLatch;
Review Comment:
This import is rejected by Cassandra's `IllegalImport` check (`.build/checkstyle.xml:104`). Production code uses `org.apache.cassandra.utils.concurrent.CountDownLatch` so waits remain simulator-interceptable; switch to that type and its `newCountDownLatch`/`decrement` API, otherwise the build fails checkstyle.
##########
src/java/org/apache/cassandra/transport/SimpleClient.java:
##########
@@ -626,6 +633,18 @@ private void configureLegacyPipeline(ChannelHandlerContext ctx)
}
}
+ private void handleGracefulDisconnect()
+ {
+ draining.set(true);
+ ChannelFuture writeFuture = lastWriteFuture;
+ if (writeFuture != null)
+ writeFuture.addListener(f -> channel.close());
+ else
+ channel.close();
Review Comment:
`lastWriteFuture` only means outbound request bytes were flushed; it does not mean the corresponding responses were received. If this event arrives while `execute(List)` is waiting for responses, closing here drops those in-flight responses and causes the caller to time out—the behavior graceful disconnect is intended to prevent. Track outstanding requests and close only after their responses complete (while rejecting newly submitted requests).
##########
src/java/org/apache/cassandra/config/Config.java:
##########
@@ -143,6 +143,10 @@ public static Set<String> splitCommaDelimited(String src)
/** Triggers automatic allocation of tokens if set, based on the provided replica count for a datacenter */
public Integer allocate_tokens_for_local_replication_factor = null;
+ public boolean graceful_disconnect_enabled = false;
+
+ public volatile DurationSpec.LongMillisecondsBound graceful_disconnect_grace_period = new DurationSpec.LongMillisecondsBound(5000);
Review Comment:
`LongMillisecondsBound` accepts zero, and no startup cross-field validation is added, so `graceful_disconnect_enabled: true` with a `0ms` period is accepted even though the JMX setter explicitly rejects non-positive periods when enabled. Add the same validation during configuration initialization so YAML and JMX enforce one contract.
##########
src/java/org/apache/cassandra/service/StorageService.java:
##########
@@ -617,7 +631,9 @@ public void stopNativeTransport(boolean force)
{
throw new IllegalStateException("No configured daemon");
}
- daemon.stopNativeTransport(force);
+ gracefulDisconnect(() -> {
+ daemon.stopNativeTransport(force);
+ });
Review Comment:
This makes `stopNativeTransport` asynchronous whenever subscribers exist. The JVM shutdown hook calls `drain(true)`, whose `shutdownClientServers()` reaches this method and then immediately continues shutting down request stages and `ScheduledExecutors.nonPeriodicTasks` (which owns the grace timeout), so controlled JVM shutdown does not honor the advertised grace period and can cancel its timeout. Start and await the graceful phase before `drain(true)` proceeds, rather than returning here asynchronously.
##########
src/java/org/apache/cassandra/service/StorageService.java:
##########
@@ -3859,9 +3898,112 @@ public String getDrainProgress()
/**
* Shuts node off to writes, empties memtables and the commit log.
*/
- public synchronized void drain() throws IOException, InterruptedException, ExecutionException
+ public void drain() throws IOException, InterruptedException, ExecutionException, TimeoutException
+ {
+ if (!drainLock.tryLock())
+ {
+ throw new IllegalStateException("Drain already in progress");
+ }
+ try
+ {
+ CountDownLatch drainComplete = new CountDownLatch(1);
+ AtomicReference<Exception> failure = new AtomicReference<>();
+ gracefulDisconnect(() -> {
+ try
+ {
+ drain(false);
+ }
+ catch (IOException | InterruptedException | ExecutionException e)
+ {
+ failure.set(e);
+ }
+ finally
+ {
+ drainComplete.countDown();
+ }
+ });
+ if (!drainComplete.await(DatabaseDescriptor.getGracefulDisconnectGracePeriod(), MILLISECONDS))
+ throw new TimeoutException("Timed out waiting for drain to complete after graceful disconnect");
+ Exception e = failure.get();
+ if (e instanceof IOException) throw (IOException) e;
+ if (e instanceof InterruptedException) throw (InterruptedException) e;
+ if (e instanceof ExecutionException) throw (ExecutionException) e;
+ if (e != null) throw new RuntimeException(e);
+ }
+ finally
+ {
+ drainLock.unlock();
+ }
+ }
+
+ private void gracefulDisconnect(Runnable defaultAction)
+ {
+ NativeTransportService nts = CassandraDaemon.instance.nativeTransportService();
+ if (nts == null || nts.getServer() == null)
+ {
+ defaultAction.run();
+ return;
+ }
+ nts.getServer().stopAcceptingNewConnections();
+ ChannelGroup channelGroup = nts.getServer().getChannelsSubscribedToGracefulDisconnect();
+ if (channelGroup == null || channelGroup.isEmpty())
+ {
+ defaultAction.run();
+ return;
+ }
+ gracefulDisconnect(defaultAction, channelGroup);
+ }
+
+ public void gracefulDisconnect(Runnable defaultAction, ChannelGroup channelGroup)
{
- drain(false);
+ if (channelGroup == null || channelGroup.isEmpty())
+ {
+ defaultAction.run();
+ return;
+ }
+ AtomicBoolean actionStarted = new AtomicBoolean(false);
+ AtomicInteger connectedChannels = new AtomicInteger(channelGroup.size());
Review Comment:
`DefaultChannelGroup` removes channels concurrently on close, so `channelGroup.size()` and the subsequent weakly consistent `forEach` are not an atomic snapshot. If a channel disappears between them and is not visited, `connectedChannels` remains too high; at timeout `channelGroup.close()` may have nothing to close, no listener reaches zero, and the shutdown action never runs. Iterate a stable channel snapshot (or derive the count only from channels actually instrumented) so every counted channel has a completion listener.
--
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
Re: [PR] CASSANDRA-21191: [CEP-59] Implementation of In-Band Connection Draining (Graceful Disconnect) [cassandra]
Posted by "AshenScribe (via GitHub)" <gi...@apache.org>.
AshenScribe commented on code in PR #4953:
URL: https://github.com/apache/cassandra/pull/4953#discussion_r3815760234
##########
src/java/org/apache/cassandra/service/StorageService.java:
##########
@@ -40,13 +40,17 @@
import java.util.UUID;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.CopyOnWriteArrayList;
+import org.apache.cassandra.utils.concurrent.CountDownLatch
Review Comment:
yup, I also skipped the import style check skip patching it now.
--
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
Re: [PR] CASSANDRA-21191: [CEP-59] Implementation of In-Band Connection Draining (Graceful Disconnect) [cassandra]
Posted by "AshenScribe (via GitHub)" <gi...@apache.org>.
AshenScribe commented on PR #4953:
URL: https://github.com/apache/cassandra/pull/4953#issuecomment-5504849581
I appreciate the newCloseFuture().awaitUninterruptibly(...) approach, I'll adapot it in my implementation.
--
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
Re: [PR] CASSANDRA-21191: [CEP-59] Implementation of In-Band Connection Draining (Graceful Disconnect) [cassandra]
Posted by "SiyaoIsHiding (via GitHub)" <gi...@apache.org>.
SiyaoIsHiding commented on code in PR #4953:
URL: https://github.com/apache/cassandra/pull/4953#discussion_r3909188075
##########
src/java/org/apache/cassandra/tools/nodetool/Drain.java:
##########
@@ -34,7 +34,8 @@ public void execute(NodeProbe probe)
try
{
probe.drain();
- } catch (IOException | InterruptedException | ExecutionException e)
+ }
+ catch (IOException | InterruptedException | ExecutionException e)
Review Comment:
What's the purpose of this change
##########
src/java/org/apache/cassandra/transport/SimpleClient.java:
##########
@@ -29,7 +29,7 @@
import java.util.Queue;
Review Comment:
Please revert the formatting changes in this file and do not move things around, e.g. that Builder, so it's easier for PR reviewers to review what's actually relevant to CEP-59
##########
src/java/org/apache/cassandra/transport/GracefulDisconnectLifecycle.java:
##########
@@ -0,0 +1,153 @@
+/*
Review Comment:
Please refer to https://github.com/SiyaoIsHiding/cassandra/blob/feature/cep-59/metrics/src/java/org/apache/cassandra/transport/Server.java
On simplier draining logic.
##########
test/unit/org/apache/cassandra/metrics/ClientMetricsTest.java:
##########
@@ -239,4 +239,20 @@ public void testConnectedClientsAndAuthMetrics() throws SSLException
assertEquals(0, clientMetrics.encryptedConnectedNativeClients.getValue().intValue());
assertEquals(0, passwordConnections.getValue().intValue());
}
+
+ @Test
+ public void testGracefulDisconnectMetrics()
Review Comment:
This metrics test doesn't cover anything in the core mechanism. Please refer to other client metrics tests.
##########
src/java/org/apache/cassandra/config/Config.java:
##########
@@ -143,6 +143,10 @@ public static Set<String> splitCommaDelimited(String src)
/** Triggers automatic allocation of tokens if set, based on the provided replica count for a datacenter */
public Integer allocate_tokens_for_local_replication_factor = null;
+ public boolean graceful_disconnect_enabled = false;
+
+ public volatile DurationSpec.LongMillisecondsBound graceful_disconnect_grace_period = new DurationSpec.LongMillisecondsBound(5000);
Review Comment:
+1
##########
src/java/org/apache/cassandra/service/NativeTransportService.java:
##########
@@ -163,6 +164,11 @@ Server getServer()
return server;
}
+ public ChannelGroup getChannelsSubscribedToGracefulDisconnect()
Review Comment:
Add
```java
// for testing purpose
```
Cuz it's not used anywhere in src.
--
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
Re: [PR] CASSANDRA-21191: [CEP-59] Implementation of In-Band Connection Draining (Graceful Disconnect) [cassandra]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #4953:
URL: https://github.com/apache/cassandra/pull/4953#discussion_r4076588181
##########
src/java/org/apache/cassandra/transport/Server.java:
##########
@@ -126,15 +128,60 @@ private Server (Builder builder)
Schema.instance.registerListener(notifier);
}
+ public ChannelGroup getChannelsSubscribedToGracefulDisconnect()
+ {
+ return connectionTracker.groups.get(Event.Type.GRACEFUL_DISCONNECT);
+ }
+
public void stop()
{
stop(false);
}
public void stop(boolean force)
{
- if (isRunning.compareAndSet(true, false))
- close(force);
+ if (isRunning.compareAndSet(true, false))
+ {
+ if (!force && DatabaseDescriptor.getGracefulDisconnectEnabled())
+ gracefulDisconnect();
Review Comment:
`isRunning` becomes false before the disconnect event is sent, so `Dispatcher.dispatch()` rejects every request decoded during the grace period with `OverloadedException` (Dispatcher.java:109-119). A request sent before the client can receive `GRACEFUL_DISCONNECT` can therefore fail instead of completing. Use a separate stopping/acceptor state, or otherwise keep dispatch enabled until subscribed clients have drained while still preventing duplicate stops.
##########
src/java/org/apache/cassandra/transport/Server.java:
##########
@@ -126,15 +128,60 @@ private Server (Builder builder)
Schema.instance.registerListener(notifier);
}
+ public ChannelGroup getChannelsSubscribedToGracefulDisconnect()
+ {
+ return connectionTracker.groups.get(Event.Type.GRACEFUL_DISCONNECT);
+ }
+
public void stop()
{
stop(false);
}
public void stop(boolean force)
{
- if (isRunning.compareAndSet(true, false))
- close(force);
+ if (isRunning.compareAndSet(true, false))
+ {
+ if (!force && DatabaseDescriptor.getGracefulDisconnectEnabled())
+ gracefulDisconnect();
+ close(force);
+ }
+ }
+
+ private void gracefulDisconnect()
+ {
+ stopAcceptingNewConnections();
+
+ ChannelGroup channelGroup = getChannelsSubscribedToGracefulDisconnect();
+ if (channelGroup.isEmpty())
+ return;
+
+ channelGroup.forEach(channel ->
+ {
+ ClientMetrics.instance.connectionsDraining.set(channelGroup.size());
+ channel.closeFuture().addListener(future -> ClientMetrics.instance.decrementConnectionsDraining());
+ }
+ );
Review Comment:
Setting the gauge from the live group inside its weakly consistent iteration races with automatic channel removal. If a channel closes after an earlier iteration but is removed before its own listener is installed, the initial count includes it but no listener decrements it, leaving `ConnectionsDraining` stale after shutdown. Snapshot the channels, set the count once, and attach one listener to every snapshot entry.
##########
doc/modules/cassandra/pages/managing/operating/graceful_disconnect.adoc:
##########
@@ -0,0 +1,91 @@
+= Graceful Disconnect — In-Band Connection Draining for Cassandra Node Shutdown
Review Comment:
This new operating guide is not referenced by `doc/modules/cassandra/nav.adoc` (the operating entries are at lines 93-118), and no other documentation links to it. As a result, the generated documentation has no navigation path to the page. Add it to the operating section of the Antora navigation.
##########
test/distributed/org/apache/cassandra/distributed/test/GracefulDisconnectTest.java:
##########
@@ -0,0 +1,488 @@
+/*
+ * 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 org.apache.cassandra.distributed.test;
+
+import java.io.IOException;
+import java.net.InetSocketAddress;
+import java.nio.charset.StandardCharsets;
+import java.util.Collections;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.TimeUnit;
+
+import org.awaitility.Awaitility;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import org.apache.cassandra.config.DatabaseDescriptor;
+import org.apache.cassandra.cql3.QueryOptions;
+import org.apache.cassandra.distributed.Cluster;
+import org.apache.cassandra.distributed.api.Feature;
+import org.apache.cassandra.metrics.ClientMetrics;
+import org.apache.cassandra.service.CassandraDaemon;
+import org.apache.cassandra.transport.Event;
+import org.apache.cassandra.transport.Message;
+import org.apache.cassandra.transport.ProtocolVersion;
+import org.apache.cassandra.transport.SimpleClient;
+import org.apache.cassandra.transport.messages.OptionsMessage;
+import org.apache.cassandra.transport.messages.QueryMessage;
+import org.apache.cassandra.transport.messages.ReadyMessage;
+import org.apache.cassandra.transport.messages.RegisterMessage;
+import org.apache.cassandra.transport.messages.ResultMessage;
+import org.apache.cassandra.transport.messages.StartupMessage;
+import org.apache.cassandra.transport.messages.SupportedMessage;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+public class GracefulDisconnectTest
+{
+
+ @BeforeClass
+ public static void setUp() throws IOException
+ {
+ DatabaseDescriptor.daemonInitialization();
+ }
+
+ public Cluster buildCluster(int nodeCount, boolean gracefulDisconnectEnabled) throws IOException
+ {
+ return Cluster
+ .build(nodeCount)
+ .withConfig(config ->
+ config
+ .with(Feature.NATIVE_PROTOCOL, Feature.GOSSIP)
+ .set("graceful_disconnect_enabled", gracefulDisconnectEnabled))
+ .start();
+ }
+
+ @Test
+ public void testGracefulDisconnectAdvertisedWhenEnabled() throws IOException
+ {
+ try (Cluster cluster = buildCluster(1, true))
+ {
+ InetSocketAddress nativeAddr = cluster.get(1).config().broadcastAddress();
+ try (SimpleClient client = SimpleClient.builder(nativeAddr.getHostString(), 9042).build())
+ {
+ client.connect(false);
+ Message.Response response = client.execute(new OptionsMessage());
+ SupportedMessage supported = (SupportedMessage) response;
+
+ assertThat(supported.supported.containsKey(StartupMessage.GRACEFUL_DISCONNECT))
+ .as("GRACEFUL_DISCONNECT should be advertised in SUPPORTED when enabled")
+ .isTrue();
+ }
+ }
+ }
+
+ @Test
+ public void testGracefulDisconnectDoesNotAdvertisedWhenNotEnabled() throws IOException
+ {
+ try (Cluster cluster = buildCluster(1, false))
+ {
+ InetSocketAddress nativeAddr = cluster.get(1).config().broadcastAddress();
+ try (SimpleClient client = SimpleClient.builder(nativeAddr.getHostString(), 9042).build())
+ {
+ client.connect(false);
+ SupportedMessage supported = (SupportedMessage) client.execute(new OptionsMessage());
+
+ assertThat(supported.supported.containsKey(StartupMessage.GRACEFUL_DISCONNECT))
+ .as("GRACEFUL_DISCONNECT should NOT be advertised in SUPPORTED when disabled")
+ .isFalse();
+ }
+ }
+ }
+
+ @Test
+ public void testSubscriptionViaREGISTER() throws IOException
+ {
+ try (Cluster cluster = buildCluster(1, true))
+ {
+ InetSocketAddress nativeAddr = cluster.get(1).config().broadcastAddress();
+ try (SimpleClient client = SimpleClient.builder(nativeAddr.getHostString(), 9042)
+ .protocolVersion(ProtocolVersion.V5)
+ .build())
+ {
+ client.connect(false);
+ Message.Response response = client.execute(
+ new RegisterMessage(Collections.singletonList(Event.Type.GRACEFUL_DISCONNECT)));
+
+ assertThat(response).isInstanceOf(ReadyMessage.class);
+
+ int subscribedCount = cluster.get(1).callOnInstance(() ->
+ CassandraDaemon.getInstanceForTesting()
+ .nativeTransportService()
+ .getChannelsSubscribedToGracefulDisconnectCount());
+
+ assertThat(subscribedCount)
+ .as("One channel should be subscribed to GRACEFUL_DISCONNECT")
+ .isEqualTo(1);
+ }
+ }
+ }
+
+ @Test
+ public void testRegisterGracefulDisconnectAcceptedOnV4() throws IOException
+ {
+ try (Cluster cluster = buildCluster(1, true))
+ {
+ InetSocketAddress nativeAddr = cluster.get(1).config().broadcastAddress();
+ try (SimpleClient client = SimpleClient.builder(nativeAddr.getHostString(), 9042).protocolVersion(ProtocolVersion.V4).build())
+ {
+ client.connect(false);
+ client.execute(new RegisterMessage(Collections.singletonList(Event.Type.GRACEFUL_DISCONNECT)));
+ int subscribedCount = cluster.get(1).callOnInstance(() ->
+ CassandraDaemon.getInstanceForTesting()
+ .nativeTransportService()
+ .getChannelsSubscribedToGracefulDisconnectCount());
+ assertThat(subscribedCount)
+ .as("V4 client should be able to subscribe")
+ .isEqualTo(1);
+ }
+ }
+ }
+
+ @Test
+ public void testNonSubscribedClientDoesNotReceiveEvent() throws IOException
+ {
+ try (Cluster cluster = buildCluster(1, true))
+ {
+ InetSocketAddress nativeAddr = cluster.get(1).config().broadcastAddress();
+ try (SimpleClient client = SimpleClient.builder(nativeAddr.getHostString(), 9042)
+ .protocolVersion(ProtocolVersion.V4)
+ .build())
+ {
+ client.connect(false);
+ int subscribedCount = cluster.get(1).callOnInstance(() ->
+ CassandraDaemon.getInstanceForTesting()
+ .nativeTransportService()
+ .getChannelsSubscribedToGracefulDisconnectCount());
+ assertThat(subscribedCount).isEqualTo(0);
+ }
+ }
+ }
+
+ @Test
+ public void testServerStopsAcceptingNewConnectionsOnDrain() throws IOException
+ {
+ try (Cluster cluster = buildCluster(1, true))
+ {
+ InetSocketAddress nativeAddr = cluster.get(1).config().broadcastAddress();
+ try (SimpleClient client = SimpleClient.builder(nativeAddr.getHostString(), 9042).build())
+ {
+ client.connect(false);
+ cluster.get(1).nodetool("drain");
+
+ assertThatThrownBy(() -> {
+ SimpleClient newClient = SimpleClient.builder(nativeAddr.getHostString(), 9042).build();
+ newClient.connect(false);
+ }).as("Server should reject new connections after stopAcceptingNewConnections")
+ .isNotNull();
+ cluster.get(1).shutdown();
+ }
+ }
+ }
+
+ @Test
+ public void testNoEventEmittedWhenDisabled() throws IOException
+ {
+ try (Cluster cluster = buildCluster(1, false))
+ {
+ InetSocketAddress nativeAddr = cluster.get(1).config().broadcastAddress();
+ try (SimpleClient client = SimpleClient.builder(nativeAddr.getHostString(), 9042).build())
+ {
+ client.connect(false);
+ client.execute(new RegisterMessage(Collections.singletonList(Event.Type.GRACEFUL_DISCONNECT)));
+
+ int subscribedCount = cluster.get(1).callOnInstance(() ->
+ CassandraDaemon.getInstanceForTesting()
+ .nativeTransportService()
+ .getChannelsSubscribedToGracefulDisconnectCount());
+
+ assertThat(subscribedCount).isEqualTo(0);
+
+ boolean disabled = cluster.get(1).callOnInstance(() -> !DatabaseDescriptor.getGracefulDisconnectEnabled());
+ assertThat(disabled).isTrue();
+ }
+ }
+ }
+
+ @Test
+ public void testDrainProceedsImmediatelyWithNoSubscribedConnections() throws IOException
+ {
+ try (Cluster cluster = buildCluster(1, true))
+ {
+ InetSocketAddress nativeAddr = cluster.get(1).config().broadcastAddress();
+ try (SimpleClient client = SimpleClient.builder(nativeAddr.getHostString(), 9042)
+ .protocolVersion(ProtocolVersion.V4)
+ .build())
+ {
+ client.connect(false);
+ cluster.get(1).nodetoolResult("drain").asserts().success();
+ cluster.get(1).shutdown();
+ }
+ }
+ }
+
+ @Test
+ public void testMultipleConnectionsCanSubscribe() throws IOException
+ {
+ try (Cluster cluster = buildCluster(1, true))
+ {
+ InetSocketAddress nativeAddr = cluster.get(1).config().broadcastAddress();
+ try (SimpleClient client1 = SimpleClient.builder(nativeAddr.getHostString(), 9042)
+ .protocolVersion(ProtocolVersion.V5)
+ .build();
+ SimpleClient client2 = SimpleClient.builder(nativeAddr.getHostString(), 9042)
+ .protocolVersion(ProtocolVersion.V5)
+ .build())
+ {
+ client1.connect(false);
+ client2.connect(false);
+
+ client1.execute(new RegisterMessage(Collections.singletonList(Event.Type.GRACEFUL_DISCONNECT)));
+ client2.execute(new RegisterMessage(Collections.singletonList(Event.Type.GRACEFUL_DISCONNECT)));
+
+ int subscribedCount = cluster.get(1).callOnInstance(() ->
+ CassandraDaemon.getInstanceForTesting()
+ .nativeTransportService()
+ .getChannelsSubscribedToGracefulDisconnectCount());
+
+ assertThat(subscribedCount).isEqualTo(2);
+ }
+ }
+ }
+
+ @Test
+ public void testInFlightQueryCompletesDuringDrain() throws Exception
+ {
+ try (Cluster cluster = buildCluster(1, true))
+ {
+ cluster.schemaChange("CREATE KEYSPACE ks WITH replication = {'class': 'SimpleStrategy', 'replication_factor': 1}");
+ cluster.schemaChange("CREATE TABLE ks.tbl (id int PRIMARY KEY, val text)");
+ cluster.get(1).executeInternal("INSERT INTO ks.tbl (id, val) VALUES (1, 'test_val')");
+
+ InetSocketAddress nativeAddr = cluster.get(1).config().broadcastAddress();
+ try (SimpleClient client = SimpleClient.builder(nativeAddr.getHostString(), 9042)
+ .protocolVersion(ProtocolVersion.V5)
+ .build())
+ {
+ client.connect(false);
+ client.execute(new RegisterMessage(Collections.singletonList(Event.Type.GRACEFUL_DISCONNECT)));
+
+ CompletableFuture<Void> drainFuture = CompletableFuture.runAsync(() -> {
+ cluster.get(1).nodetool("drain");
+ });
+
+ Message.Response response = client.execute(new org.apache.cassandra.transport.messages.QueryMessage(
Review Comment:
This does not establish that the query is in flight when draining begins: `runAsync` only schedules nodetool, so the query may finish before shutdown starts, or shutdown may win and reject it. The test can pass without exercising the guarantee and is timing-dependent. Block or latch a request after server dispatch, then start drain and release the request.
##########
src/java/org/apache/cassandra/transport/SimpleClient.java:
##########
@@ -322,8 +345,14 @@ public Message.Response execute(Message.Request request)
return execute(request, true);
}
+
public Message.Response execute(Message.Request request, boolean throwOnErrorResponse)
{
+ if (draining.get())
+ throw new RuntimeException("Connection is draining (GRACEFUL_DISCONNECT received)");
+
+ CompletableFuture<Void> requestCompletion = new CompletableFuture<>();
+ inFlight = requestCompletion;
Review Comment:
The draining check and publication of `requestCompletion` are not atomic. If the event arrives between these lines, the handler observes the previous completed future and schedules an immediate close while this request proceeds to write. Also, the pipelined `execute(List)` path never updates `inFlight`, so it is always closed immediately. Track all execute overloads with one synchronized/atomic in-flight counter so closure occurs only after every request that passed admission completes.
--
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
Re: [PR] CASSANDRA-21191: [CEP-59] Implementation of In-Band Connection Draining (Graceful Disconnect) [cassandra]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #4953:
URL: https://github.com/apache/cassandra/pull/4953#discussion_r4286316784
##########
src/java/org/apache/cassandra/transport/Server.java:
##########
@@ -120,21 +127,64 @@ private Server (Builder builder)
dispatcher);
EventNotifier notifier = builder.eventNotifier != null ? builder.eventNotifier : new EventNotifier();
- connectionTracker = new ConnectionTracker(isRunning::get);
+ connectionTracker = new ConnectionTracker(acceptingRequests::get);
notifier.registerConnectionTracker(connectionTracker);
StorageService.instance.register(notifier);
Schema.instance.registerListener(notifier);
}
+ public ChannelGroup getChannelsSubscribedToGracefulDisconnect()
+ {
+ return connectionTracker.groups.get(Event.Type.GRACEFUL_DISCONNECT);
+ }
+
public void stop()
{
stop(false);
}
public void stop(boolean force)
{
- if (isRunning.compareAndSet(true, false))
- close(force);
+ if (isRunning.compareAndSet(true, false))
+ {
+ if (!force && DatabaseDescriptor.getGracefulDisconnectEnabled())
+ gracefulDisconnect();
Review Comment:
`gracefulDisconnect()` is invoked twice for a normal stop when the feature is enabled: once in `stop(false)` and again in `close(false)`. This can emit duplicate events, double-register close listeners, and corrupt draining/forced-disconnect metrics (including driving `ConnectionsDraining` negative). Remove one of the invocations (typically: let `close(false)` own the graceful disconnect flow and do not call it from `stop(boolean)`), and ensure the draining flow is executed at most once per stop lifecycle.
##########
src/java/org/apache/cassandra/transport/Server.java:
##########
@@ -202,6 +269,11 @@ private void close(boolean force)
{
if (!force)
{
+ if (DatabaseDescriptor.getGracefulDisconnectEnabled())
+ gracefulDisconnect();
+ else
+ stopAcceptingNewConnections();
Review Comment:
`gracefulDisconnect()` is invoked twice for a normal stop when the feature is enabled: once in `stop(false)` and again in `close(false)`. This can emit duplicate events, double-register close listeners, and corrupt draining/forced-disconnect metrics (including driving `ConnectionsDraining` negative). Remove one of the invocations (typically: let `close(false)` own the graceful disconnect flow and do not call it from `stop(boolean)`), and ensure the draining flow is executed at most once per stop lifecycle.
##########
src/java/org/apache/cassandra/transport/Server.java:
##########
@@ -355,11 +433,13 @@ void closeAll()
int countConnectedClients()
{
/*
- - When server is running: allChannels contains all clients' connections (channels)
- plus one additional channel used for the server's own bootstrap.
- - When server is stopped: the size is 0
+ Count only channels associated with a ServerConnection. The server's
+ bootstrap channel is not associated with a ServerConnection and is
+ therefore excluded.
*/
- return allChannels.size() != 0 ? allChannels.size() - 1 : 0;
+ return (int) allChannels.stream()
+ .filter(channel -> channel.pipeline().get(String.valueOf(ServerConnection.class)) != null)
Review Comment:
`ChannelPipeline.get(String)` looks up a handler by *name*, but `String.valueOf(ServerConnection.class)` yields `"class org.apache.cassandra.transport.ServerConnection"`, which is very unlikely to match the pipeline handler name. This will typically return `null` for all channels and make `countConnectedClients()` always return 0. Use the `get(Class<? extends ChannelHandler>)` overload (or otherwise track client channels explicitly) so the filter correctly identifies channels associated with a `ServerConnection`.
##########
src/java/org/apache/cassandra/metrics/ClientMetrics.java:
##########
@@ -173,6 +179,16 @@ public void markProtocolException()
protocolException.mark();
}
+ public void decrementConnectionsDraining()
+ {
+ connectionsDraining.decrementAndGet();
Review Comment:
The current API mixes `setConnectionsDraining(size)` with per-channel `decrementConnectionsDraining()` callbacks. If draining is initiated more than once (or if close callbacks outlive a reset), the gauge can go negative or otherwise become inconsistent. Consider switching to a purely delta-based approach (increment on drain registration, decrement on channel close) and/or clamp at zero in `decrementConnectionsDraining()`. This is especially important given the current double-invocation risk in the server stop path.
##########
src/java/org/apache/cassandra/transport/Server.java:
##########
@@ -149,12 +199,29 @@ public synchronized void start()
// Configure the server.
ChannelFuture bindFuture = pipelineConfigurator.initializeChannel(workerGroup, socket, connectionFactory);
+ bindChannel = bindFuture.channel();
if (!bindFuture.awaitUninterruptibly().isSuccess())
throw new IllegalStateException(String.format("Failed to bind port %d on %s.", socket.getPort(), socket.getAddress().getHostAddress()),
bindFuture.cause());
connectionTracker.allChannels.add(bindFuture.channel());
isRunning.set(true);
+ acceptingRequests.set(true);
+ }
+
+ /**
+ * Permanently stops the native transport acceptor from accepting new connections.
+ * This does NOT reopen on its own — a full native transport restart is required
+ * to accept new connections again after this is called.
+ */
+ public void stopAcceptingNewConnections()
+ {
+ 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();
Review Comment:
Calling `close().syncUninterruptibly()` can deadlock if `stopAcceptingNewConnections()` is ever invoked on the acceptor channel’s event-loop thread. Prefer closing asynchronously (e.g., `close()` + listener / await from a non-event-loop thread), or guard with an `inEventLoop()` check and avoid blocking in that case.
##########
conf/cassandra_latest.yaml:
##########
@@ -1087,6 +1087,20 @@ native_transport_allow_older_protocols: true
# native_transport_rate_limiting_enabled: false
# native_transport_max_requests_per_second: 1000000
+# When enabled, nodes will signal connected clients before shutting down,
+# allowing in-flight requests to complete without client-visible timeouts.
+# This applies to intentional shutdowns (nodetool drain, rolling restarts,
+# controlled JVM shutdown). Clients must subscribe to the GRACEFUL_DISCONNECT
+# event via REGISTER to benefit from this behavior.
+#
+# Requires driver support for the GRACEFUL_DISCONNECT event type.
+# See: doc/modules/cassandra/pages/managing/operating/graceful_disconnect.adoc
+
+graceful_disconnect_enabled: true
Review Comment:
This enables graceful disconnect in `cassandra_latest.yaml` by default, while `conf/cassandra.yaml` and the code default indicate the feature defaults to disabled. If `cassandra_latest.yaml` is used as a “default”/reference config by operators or tooling, this can unintentionally change behavior (drain timing + forced disconnects) without explicit opt-in. Consider commenting this out (matching `cassandra.yaml`) or adding a clearer rationale in the file for why the “latest” config enables it by default.
##########
test/unit/org/apache/cassandra/transport/GracefulDisconnectMetricsTest.java:
##########
@@ -0,0 +1,131 @@
+/*
+ * 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 org.apache.cassandra.transport;
+
+import java.net.InetAddress;
+import java.net.ServerSocket;
+import java.util.Collections;
+import java.util.concurrent.TimeUnit;
+
+import org.awaitility.Awaitility;
+import org.junit.Before;
+import org.junit.Test;
+
+import org.apache.cassandra.auth.AllowAllAuthenticator;
+import org.apache.cassandra.auth.AllowAllAuthorizer;
+import org.apache.cassandra.auth.AllowAllNetworkAuthorizer;
+import org.apache.cassandra.config.DatabaseDescriptor;
+import org.apache.cassandra.metrics.ClientMetrics;
+import org.apache.cassandra.transport.messages.RegisterMessage;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+public class GracefulDisconnectMetricsTest
+{
+
+ private InetAddress address;
+ private int port;
+
+ @Before
+ public void setup() throws Exception
+ {
+ DatabaseDescriptor.toolInitialization();
+ DatabaseDescriptor.setAuthenticator(new AllowAllAuthenticator());
+ DatabaseDescriptor.setAuthorizer(new AllowAllAuthorizer());
+ DatabaseDescriptor.setNetworkAuthorizer(new AllowAllNetworkAuthorizer());
+
+ DatabaseDescriptor.getRawConfig().graceful_disconnect_enabled = true;
+ DatabaseDescriptor.setGracefulDisconnectGracePeriod(1000);
+
+ address = InetAddress.getLoopbackAddress();
+ try (ServerSocket serverSocket = new ServerSocket(0))
+ {
+ port = serverSocket.getLocalPort();
+ }
+ Thread.sleep(250);
+ }
+
+ private Server startServer()
+ {
+ Server server = new Server.Builder().withHost(address).withPort(port).build();
+ server.start();
+ ClientMetrics.instance.init(server);
+ return server;
+ }
+
+ private SimpleClient registeredClient(boolean cooperative) throws Exception
+ {
+ SimpleClient.Builder builder = SimpleClient.builder(address.getHostAddress(), port)
+ .protocolVersion(ProtocolVersion.V5);
+ if (!cooperative)
+ builder.ignoreGracefulDisconnect();
+
+ SimpleClient client = builder.build();
+ client.connect(false);
+ client.execute(new RegisterMessage(Collections.singletonList(Event.Type.GRACEFUL_DISCONNECT)));
+ return client;
+ }
+
+ @Test
+ public void cooperativeClientDrainsAndMetricsReturnToBaseline() throws Exception
+ {
+ Server server = startServer();
+ long baselineForced = ClientMetrics.instance.getForcedDisconnectsCount();
+ int baselineDraining = ClientMetrics.instance.getConnectionsDraining();
+
+ try (SimpleClient client = registeredClient(true))
+ {
+ server.stop();
+
+ assertThat(ClientMetrics.instance.getConnectionsDraining())
+ .as("connectionsDraining should settle back to its pre-drain baseline")
+ .isEqualTo(baselineDraining);
+ assertThat(ClientMetrics.instance.getForcedDisconnectsCount())
+ .as("a cooperative disconnect must not be counted as forced")
+ .isEqualTo(baselineForced);
Review Comment:
These assertions are executed immediately after `server.stop()`, but the draining metric updates and channel closes are inherently asynchronous. This makes the test prone to flakiness under load/slow CI. Use `Awaitility` (as in the other test) to wait until `connectionsDraining` and `forcedDisconnects` reach the expected values before asserting.
##########
test/distributed/org/apache/cassandra/distributed/test/GracefulDisconnectTest.java:
##########
@@ -0,0 +1,506 @@
+/*
+ * 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 org.apache.cassandra.distributed.test;
+
+import java.io.IOException;
+import java.net.InetSocketAddress;
+import java.nio.charset.StandardCharsets;
+import java.util.Collections;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.TimeUnit;
+
+import org.awaitility.Awaitility;
+import org.junit.BeforeClass;
+import org.junit.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.cassandra.config.DatabaseDescriptor;
+import org.apache.cassandra.cql3.QueryOptions;
+import org.apache.cassandra.distributed.Cluster;
+import org.apache.cassandra.distributed.api.Feature;
+import org.apache.cassandra.metrics.ClientMetrics;
+import org.apache.cassandra.service.CassandraDaemon;
+import org.apache.cassandra.transport.Event;
+import org.apache.cassandra.transport.Message;
+import org.apache.cassandra.transport.ProtocolVersion;
+import org.apache.cassandra.transport.SimpleClient;
+import org.apache.cassandra.transport.messages.OptionsMessage;
+import org.apache.cassandra.transport.messages.QueryMessage;
+import org.apache.cassandra.transport.messages.ReadyMessage;
+import org.apache.cassandra.transport.messages.RegisterMessage;
+import org.apache.cassandra.transport.messages.ResultMessage;
+import org.apache.cassandra.transport.messages.StartupMessage;
+import org.apache.cassandra.transport.messages.SupportedMessage;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+public class GracefulDisconnectTest
+{
+
+ private static final String GRACE_PERIOD = "4s";
+ private static final Logger log = LoggerFactory.getLogger(GracefulDisconnectTest.class);
+
+ @BeforeClass
+ public static void setUp() throws IOException
+ {
+ DatabaseDescriptor.daemonInitialization();
+ }
+
+ public Cluster buildCluster(int nodeCount, boolean gracefulDisconnectEnabled) throws IOException
+ {
+ return Cluster
+ .build(nodeCount)
+ .withConfig(config ->
+ config
+ .with(Feature.NATIVE_PROTOCOL, Feature.GOSSIP)
+ .set("graceful_disconnect_enabled", gracefulDisconnectEnabled))
+ .start();
+ }
+
+ @Test
+ public void testGracefulDisconnectAdvertisedWhenEnabled() throws IOException
+ {
+ try (Cluster cluster = buildCluster(1, true))
+ {
+ InetSocketAddress nativeAddr = cluster.get(1).config().broadcastAddress();
+ try (SimpleClient client = SimpleClient.builder(nativeAddr.getHostString(), 9042).build())
+ {
+ client.connect(false);
+ Message.Response response = client.execute(new OptionsMessage());
+ SupportedMessage supported = (SupportedMessage) response;
+
+ assertThat(supported.supported.containsKey(StartupMessage.GRACEFUL_DISCONNECT))
+ .as("GRACEFUL_DISCONNECT should be advertised in SUPPORTED when enabled")
+ .isTrue();
+ }
+ }
+ }
+
+ @Test
+ public void testGracefulDisconnectDoesNotAdvertisedWhenNotEnabled() throws IOException
Review Comment:
The method name is grammatically incorrect (`DoesNotAdvertised`). Rename to something like `testGracefulDisconnectIsNotAdvertisedWhenDisabled` / `testGracefulDisconnectDoesNotAdvertiseWhenNotEnabled` to keep test names clear and consistent.
##########
src/java/org/apache/cassandra/transport/messages/OptionsMessage.java:
##########
@@ -65,6 +66,7 @@ protected Message.Response execute(QueryState state, Dispatcher.RequestTime requ
cqlVersions.add(QueryProcessor.CQL_VERSION.toString());
List<String> compressions = new ArrayList<String>();
+ final List<String> gracefulDisconnect = List.of("true");
Review Comment:
This creates a new single-element list for every OPTIONS execution. Since the contents are constant, consider using a static constant (e.g., a `private static final List<String>`), reducing per-request allocations on a hot path.
##########
src/java/org/apache/cassandra/metrics/ClientMetrics.java:
##########
@@ -335,4 +354,21 @@ public void queueTime(long value, TimeUnit unit)
{
queueTime.update(value, unit);
}
+
+ @VisibleForTesting
+ public long getForcedDisconnectsCount()
+ {
+ return forcedDisconnects.getCount();
+ }
+
+ @VisibleForTesting
+ public int getConnectionsDraining()
+ {
+ return connectionsDraining.get();
+ }
+
+ public void setConnectionsDraining(int draining)
+ {
+ connectionsDraining.set(draining);
Review Comment:
The current API mixes `setConnectionsDraining(size)` with per-channel `decrementConnectionsDraining()` callbacks. If draining is initiated more than once (or if close callbacks outlive a reset), the gauge can go negative or otherwise become inconsistent. Consider switching to a purely delta-based approach (increment on drain registration, decrement on channel close) and/or clamp at zero in `decrementConnectionsDraining()`. This is especially important given the current double-invocation risk in the server stop path.
##########
src/java/org/apache/cassandra/config/DatabaseDescriptor.java:
##########
@@ -618,6 +618,9 @@ private static void applySimpleConfig()
InetAddressAndPort.initializeDefaultPort(getStoragePort());
validateUpperBoundStreamingConfig();
+
Review Comment:
Line 621 contains trailing whitespace on an otherwise blank line, which may trip formatting/checkstyle rules in this codebase. Remove the trailing spaces.
--
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
Re: [PR] CASSANDRA-21191: [CEP-59] Implementation of In-Band Connection Draining (Graceful Disconnect) [cassandra]
Posted by "AshenScribe (via GitHub)" <gi...@apache.org>.
AshenScribe commented on code in PR #4953:
URL: https://github.com/apache/cassandra/pull/4953#discussion_r4280047779
##########
src/java/org/apache/cassandra/transport/Server.java:
##########
@@ -126,15 +128,60 @@ private Server (Builder builder)
Schema.instance.registerListener(notifier);
}
+ public ChannelGroup getChannelsSubscribedToGracefulDisconnect()
+ {
+ return connectionTracker.groups.get(Event.Type.GRACEFUL_DISCONNECT);
+ }
+
public void stop()
{
stop(false);
}
public void stop(boolean force)
{
- if (isRunning.compareAndSet(true, false))
- close(force);
+ if (isRunning.compareAndSet(true, false))
+ {
+ if (!force && DatabaseDescriptor.getGracefulDisconnectEnabled())
+ gracefulDisconnect();
+ close(force);
+ }
+ }
+
+ private void gracefulDisconnect()
+ {
+ stopAcceptingNewConnections();
+
+ ChannelGroup channelGroup = getChannelsSubscribedToGracefulDisconnect();
+ if (channelGroup.isEmpty())
+ return;
+
+ channelGroup.forEach(channel ->
+ {
+ ClientMetrics.instance.connectionsDraining.set(channelGroup.size());
+ channel.closeFuture().addListener(future -> ClientMetrics.instance.decrementConnectionsDraining());
+ }
+ );
Review Comment:
-1 on this approach. ClientMetrics are either package-private or private and expose functions to modify them.
--
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
Re: [PR] CASSANDRA-21191: [CEP-59] Implementation of In-Band Connection Draining (Graceful Disconnect) [cassandra]
Posted by "AshenScribe (via GitHub)" <gi...@apache.org>.
AshenScribe commented on PR #4953:
URL: https://github.com/apache/cassandra/pull/4953#issuecomment-6185729337
I'll re read the previous review again and see which one are missing.
--
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
Re: [PR] CASSANDRA-21191: [CEP-59] Implementation of In-Band Connection Draining (Graceful Disconnect) [cassandra]
Posted by "AshenScribe (via GitHub)" <gi...@apache.org>.
AshenScribe commented on PR #4953:
URL: https://github.com/apache/cassandra/pull/4953#issuecomment-6164340185
yes I replaced a test case to make it more deterministic, since last time.
as of `graceful_disconnect_enabled` in `cassandra_latest.yaml` I guess `true` is better as graceful disconnect doesn't break anything and its a nice to have feature.
--
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
Re: [PR] CASSANDRA-21191: [CEP-59] Implementation of In-Band Connection Draining (Graceful Disconnect) [cassandra]
Posted by "SiyaoIsHiding (via GitHub)" <gi...@apache.org>.
SiyaoIsHiding commented on code in PR #4953:
URL: https://github.com/apache/cassandra/pull/4953#discussion_r4340712020
##########
doc/modules/cassandra/pages/managing/operating/graceful_disconnect.adoc:
##########
@@ -0,0 +1,91 @@
+= Graceful Disconnect — In-Band Connection Draining for Cassandra Node Shutdown
Review Comment:
If I remember correctly, Abe suggested to link to a doc, then you wrote a doc right next to a .java file, then me and Mick suggested a doc next to a .java file is for Cassandra developers to read, a doc in cassandra/doc is for Cassandra users/operators to read. So we end up here?
I also think such feature, involving two configs that a Cassandra operator needs to understand, deserve a page in the manual, although this page in the manual can largely copy from the CEP wiki page.
A link to the JIRA ticket alone isn't enough. JIRA ticket and wiki is mainly for Cassadra developers. Our readers are Cassandra users.
--
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
Re: [PR] CASSANDRA-21191: [CEP-59] Implementation of In-Band Connection Draining (Graceful Disconnect) [cassandra]
Posted by "SiyaoIsHiding (via GitHub)" <gi...@apache.org>.
SiyaoIsHiding commented on code in PR #4953:
URL: https://github.com/apache/cassandra/pull/4953#discussion_r4341292588
##########
doc/modules/cassandra/pages/managing/operating/graceful_disconnect.adoc:
##########
@@ -0,0 +1,91 @@
+= Graceful Disconnect — In-Band Connection Draining for Cassandra Node Shutdown
Review Comment:
A lot of things in manual is info that already exists in codebase/jira. Manual is user facing, jira is mainly Cassandra developers facing.
--
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