Fix nodetool enable/disablebinary to correctly set rpc readiness in g…

…ossip patch by Stefan Miklosovic; reviewed by Brandon Williams for CASSANDRA-18935
apache · Oct 24, 2023 · b51ee83 · b51ee83
1 parent f27c6c8
commit b51ee83
Show file tree

Hide file tree

Showing 3 changed files with 35 additions and 3 deletions.
diff --git a/CHANGES.txt b/CHANGES.txt
@@ -1,4 +1,5 @@
 3.0.30
+ * Fix nodetool enable/disablebinary to correctly set rpc readiness in gossip (CASSANDRA-18935)
  * Implement the logic in bin/stop-server (CASSANDRA-18838) 
  * Upgrade snappy-java to 1.1.10.4 (CASSANDRA-18878)
  * Add cqlshrc.sample and credentials.sample into Debian package (CASSANDRA-18818)

diff --git a/src/java/org/apache/cassandra/service/CassandraDaemon.java b/src/java/org/apache/cassandra/service/CassandraDaemon.java
@@ -548,10 +548,7 @@ private void startClientTransports()
     {
         String nativeFlag = System.getProperty("cassandra.start_native_transport");
         if ((nativeFlag != null && Boolean.parseBoolean(nativeFlag)) || (nativeFlag == null && DatabaseDescriptor.startNativeTransport()))
-        {
             startNativeTransport();
-            StorageService.instance.setRpcReady(true);
-        }
         else
             logger.info("Not starting native transport as requested. Use JMX (StorageService->startNativeTransport()) or nodetool (enablebinary) to start it");
 
@@ -714,13 +711,22 @@ public void startNativeTransport()
         if (nativeTransportService == null)
             throw new IllegalStateException("setup() must be called first for CassandraDaemon");
 
+        // this iterates over a collection of servers and returns true if one of them is started
+        boolean alreadyRunning = nativeTransportService.isRunning();
+
+        // this might in practice start all servers which are not started yet
         nativeTransportService.start();
+
+        // interact with gossip only in case if no server was started before to signal they are started now
+        if (!alreadyRunning)
+            StorageService.instance.setRpcReady(true);
     }
 
     public void stopNativeTransport()
     {
         if (nativeTransportService != null)
         {
+            StorageService.instance.setRpcReady(false);
             nativeTransportService.stop();
         }
     }

diff --git a/test/distributed/org/apache/cassandra/distributed/test/NativeProtocolTest.java b/test/distributed/org/apache/cassandra/distributed/test/NativeProtocolTest.java
@@ -33,6 +33,7 @@
 import org.apache.cassandra.distributed.api.IIsolatedExecutor;
 import org.apache.cassandra.distributed.impl.RowUtil;
 import org.apache.cassandra.service.StorageService;
+import org.apache.cassandra.utils.FBUtilities;
 
 import static org.apache.cassandra.distributed.action.GossipHelper.withProperty;
 import static org.apache.cassandra.distributed.api.Feature.GOSSIP;
@@ -119,4 +120,28 @@ public void restartTransportOnGossippingOnlyMember() throws Throwable
                                                            () -> StorageService.instance.isNativeTransportRunning()));
         }
     }
+
+    @Test
+    public void testBinaryReflectsRpcReadiness() throws Throwable
+    {
+        try (Cluster cluster = builder().withNodes(1)
+                                        .withConfig(config -> config.with(NETWORK, GOSSIP, NATIVE_PROTOCOL)
+                                                                    .set("start_native_transport", "false"))
+                                        .start())
+        {
+            IInvokableInstance i = cluster.get(1);
+
+            // rpc is false when native transport is not enabled
+            assertFalse(i.callOnInstance((IIsolatedExecutor.SerializableCallable<Boolean>) () -> StorageService.instance.isNativeTransportRunning()));
+            assertFalse(i.callOnInstance((IIsolatedExecutor.SerializableCallable<Boolean>) () -> StorageService.instance.isRpcReady(FBUtilities.getBroadcastAddress())));
+
+            // but if we enable it, e.g. by nodetool enablebinary, rpc will be enabled
+            i.runOnInstance((IIsolatedExecutor.SerializableRunnable) () -> StorageService.instance.startNativeTransport());
+            assertTrue(i.callOnInstance((IIsolatedExecutor.SerializableCallable<Boolean>) () -> StorageService.instance.isRpcReady(FBUtilities.getBroadcastAddress())));
+
+            // by calling e.g. nodetool disablebinary, rpc will be set to false again
+            i.runOnInstance((IIsolatedExecutor.SerializableRunnable) () -> StorageService.instance.stopNativeTransport());
+            assertFalse(i.callOnInstance((IIsolatedExecutor.SerializableCallable<Boolean>) () -> StorageService.instance.isRpcReady(FBUtilities.getBroadcastAddress())));
+        }
+    }
 }